Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-39150] - API stabilization && compliance with the compatibility policy #125

Conversation

oleg-nenashev
Copy link
Member

In #122 @kohsuke merged the pull request without waiting for the feedback from reviewers. It effectively introduced new public API in the stabilization branch.

I also used my chance to review the code deeply and realised that IOException handling is far from ideal for the production code. Hence I propose to change it as well

@reviewbybees

*/
public void dumpDiagnostics(PrintWriter w) throws IOException {
@Restricted(NoExternalUse.class)
public void dumpDiagnostics(@Nonnull PrintWriter w) throws IOException {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe IOException is YAGNI, but I would like to keep it since one may override Channel implementation and start throwing something. The current implementation does not throw exceptions

*/
public void dumpDiagnostics(PrintWriter w) throws IOException {
@Restricted(NoExternalUse.class)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not effectively prevent from usage in libraries, which do not invoke accmod checks in the build process. Is it fine enough?

@abayer
Copy link
Member

abayer commented Oct 24, 2016

🐝

@ghost
Copy link

ghost commented Oct 24, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

throw (Error)ex;
}
logger.log(Level.WARNING,
String.format("Cannot domp diagmostics for the channel %s", ch.getName()), ex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

domp -> dump

@rsandell
Copy link
Member

one 🐜

@oleg-nenashev
Copy link
Member Author

And also some additional FUN to handle in the merged PR.
@rsandell Should I address your comments in #122 here?

@rsandell
Copy link
Member

@oleg-nenashev my comments where more like questions as I don't know if an int is enough for long lived connections etc.
And the JMX this is more a nice to have since you then could use something like collectd et.al. for nice supervision and alarm handling. But it is probably a future addon in that case.

ch.dumpDiagnostics(w);
if (ch != null) {
try {
ch.dumpDiagnostics(w);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are performance concerned, I would recommend adding

if (w.checkError()) {
  return; 
}

as no sense continuing if PrintWriter has already suppressed the IOE

throw (Error)ex;
}
logger.log(Level.WARNING,
String.format("Cannot dump diagnostics for the channel %s", ch.getName()), ex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably send this to the print writer too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@stephenc
Copy link
Member

🐝

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐝

// Check if we can still write the output
if (w.checkError()) {
logger.log(Level.WARNING,
String.format("Cannot dump diagnostics for all channels, because output stream encontered an error. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems spelling in my NetBeans does not work anymore :(

ch.dumpDiagnostics(w);
} catch (Throwable ex) {
if (ex instanceof Error) {
throw (Error)ex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily helpful. (For ThreadDeath, yes; for LinkageError, generally no; for OutOfMemoryError, probably irrelevant.)

@oleg-nenashev
Copy link
Member Author

@reviewbybees done.
I'll fix the typo during the merge

@oleg-nenashev oleg-nenashev merged commit 9dc9317 into jenkinsci:stable-2.x Oct 25, 2016
oleg-nenashev added a commit to oleg-nenashev/jenkins that referenced this pull request Nov 9, 2016
The change introduces one serious bugfix (JENKINS-39596) and a bunch of various diagnostics improvements.

Bugfixes:

* [JENKINS-39596](https://issues.jenkins-ci.org/browse/JENKINS-39596) -
Jenkins URL in `hudson.remoting.Engine` was always `null` since `3.0`.
It was causing connection failures of Jenkins JNLP agents when using Java Web Start.
([PR jenkinsci#131](jenkinsci/remoting#131))
* [JENKINS-39617](https://issues.jenkins-ci.org/browse/JENKINS-39617) -
`hudson.remoting.Engine` was failing to establish connection if one of the URLs parameter in parameters was malformed.
([PR jenkinsci#131](jenkinsci/remoting#131))

Improvements:

* [JENKINS-39150](https://issues.jenkins-ci.org/browse/JENKINS-39150) -
Add logic for dumping diagnostics across all the channels.
([PR jenkinsci#122](jenkinsci/remoting#122), [PR jenkinsci#125](jenkinsci/remoting#125))
* [JENKINS-39543](https://issues.jenkins-ci.org/browse/JENKINS-39543) -
Improve the caller/callee correlation diagnostics in thread dumps.
([PR jenkinsci#119](jenkinsci/remoting#119))
* [JENKINS-39290](https://issues.jenkins-ci.org/browse/JENKINS-39290) -
Add the `org.jenkinsci.remoting.nio.NioChannelHub.disabled` flag for disabling NIO (mostly for debugging purposes).
([PR jenkinsci#123](jenkinsci/remoting#123))
* [JENKINS-38692](https://issues.jenkins-ci.org/browse/JENKINS-38692) -
Add extra logging to help diagnosing `IOHub` Thread spikes.
([PR #116](jenkinsci/remoting#116))
* [JENKINS-39289](https://issues.jenkins-ci.org/browse/JENKINS-39289) -
 When a proxy fails, report what caused the channel to go down.
([PR jenkinsci#128](jenkinsci/remoting#128))
oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Nov 10, 2016
The change introduces one serious bugfix (JENKINS-39596) and a bunch of various diagnostics improvements.

Bugfixes:

* [JENKINS-39596](https://issues.jenkins-ci.org/browse/JENKINS-39596) -
Jenkins URL in `hudson.remoting.Engine` was always `null` since `3.0`.
It was causing connection failures of Jenkins JNLP agents when using Java Web Start.
([PR #131](jenkinsci/remoting#131))
* [JENKINS-39617](https://issues.jenkins-ci.org/browse/JENKINS-39617) -
`hudson.remoting.Engine` was failing to establish connection if one of the URLs parameter in parameters was malformed.
([PR #131](jenkinsci/remoting#131))

Improvements:

* [JENKINS-39150](https://issues.jenkins-ci.org/browse/JENKINS-39150) -
Add logic for dumping diagnostics across all the channels.
([PR #122](jenkinsci/remoting#122), [PR #125](jenkinsci/remoting#125))
* [JENKINS-39543](https://issues.jenkins-ci.org/browse/JENKINS-39543) -
Improve the caller/callee correlation diagnostics in thread dumps.
([PR #119](jenkinsci/remoting#119))
* [JENKINS-39290](https://issues.jenkins-ci.org/browse/JENKINS-39290) -
Add the `org.jenkinsci.remoting.nio.NioChannelHub.disabled` flag for disabling NIO (mostly for debugging purposes).
([PR #123](jenkinsci/remoting#123))
* [JENKINS-38692](https://issues.jenkins-ci.org/browse/JENKINS-38692) -
Add extra logging to help diagnosing `IOHub` Thread spikes.
([PR #116](jenkinsci/remoting#116))
* [JENKINS-39289](https://issues.jenkins-ci.org/browse/JENKINS-39289) -
 When a proxy fails, report what caused the channel to go down.
([PR #128](jenkinsci/remoting#128))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants