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

oleg-nenashev · 2016-10-24T11:16:03Z

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

…anch

oleg-nenashev · 2016-10-24T11:17:02Z

src/main/java/hudson/remoting/Channel.java

     */
-    public void dumpDiagnostics(PrintWriter w) throws IOException {
+    @Restricted(NoExternalUse.class)
+    public void dumpDiagnostics(@Nonnull PrintWriter w) throws IOException {


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

oleg-nenashev · 2016-10-24T11:19:45Z

src/main/java/hudson/remoting/Channel.java

     */
-    public void dumpDiagnostics(PrintWriter w) throws IOException {
+    @Restricted(NoExternalUse.class)


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

abayer · 2016-10-24T11:23:20Z

🐝

ghost · 2016-10-24T11:24:38Z

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.

rsandell · 2016-10-24T11:31:51Z

src/main/java/hudson/remoting/Channel.java

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


domp -> dump

rsandell · 2016-10-24T11:32:35Z

one 🐜

oleg-nenashev · 2016-10-24T11:36:21Z

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

rsandell · 2016-10-24T11:56:00Z

@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.

stephenc · 2016-10-24T12:05:48Z

src/main/java/hudson/remoting/Channel.java

-                ch.dumpDiagnostics(w);
+            if (ch != null) {
+                try {
+                    ch.dumpDiagnostics(w);


If we are performance concerned, I would recommend adding

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

as no sense continuing if PrintWriter has already suppressed the IOE

stephenc · 2016-10-24T12:06:34Z

src/main/java/hudson/remoting/Channel.java

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


should probably send this to the print writer too

…tephenc

…tephenc

stephenc · 2016-10-24T13:10:15Z

🐝

@oleg-nenashev

Addresses comments from @oleg-nenashev and @rsandell

…with jenkinsci#109

jglick

🐝

jglick · 2016-10-24T20:51:05Z

src/main/java/hudson/remoting/Channel.java

+            // 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. "


Seems spelling in my NetBeans does not work anymore :(

jglick · 2016-10-24T20:51:52Z

src/main/java/hudson/remoting/Channel.java

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


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

oleg-nenashev · 2016-10-25T07:57:30Z

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

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))

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))

oleg-nenashev added 2 commits October 24, 2016 13:11

[JENKINS-39150] - Restrict the newly introduced APIs in stable-2.x br…

6683c0e

…anch

[JENKINS-39150] - Fix error processing in the diagnostics dump API

11695c3

oleg-nenashev commented Oct 24, 2016

View reviewed changes

rsandell reviewed Oct 24, 2016

View reviewed changes

[JENKINS-39150] - Fix Typos

2942de3

stephenc reviewed Oct 24, 2016

View reviewed changes

[JENKINS-39150] - Even more diagnostics according to comments from @s…

6b5c45e

…tephenc

oleg-nenashev added 2 commits October 24, 2016 15:17

[JENKINS-39150] - Statistic counters are long volatile now

3ef439f

Addresses comments from @oleg-nenashev and @rsandell

[JENKINS-39150] - Address comment from @oleg-nenashev about conflict …

53c1988

…with jenkinsci#109

jglick approved these changes Oct 24, 2016

View reviewed changes

[JENKINS-39150] - Fix another typo

43c2fca

oleg-nenashev added the hacktoberfest label Oct 25, 2016

oleg-nenashev merged commit 9dc9317 into jenkinsci:stable-2.x Oct 25, 2016

oleg-nenashev mentioned this pull request Nov 9, 2016

Update remoting to 2.31 in the Jenkins core jenkinsci/jenkins#2628

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oleg-nenashev commented Oct 24, 2016

oleg-nenashev Oct 24, 2016

oleg-nenashev Oct 24, 2016

abayer commented Oct 24, 2016

ghost commented Oct 24, 2016

rsandell Oct 24, 2016

rsandell commented Oct 24, 2016

oleg-nenashev commented Oct 24, 2016

rsandell commented Oct 24, 2016

stephenc Oct 24, 2016

stephenc Oct 24, 2016

oleg-nenashev Oct 24, 2016

stephenc commented Oct 24, 2016

jglick left a comment

jglick Oct 24, 2016

oleg-nenashev Oct 25, 2016

jglick Oct 24, 2016

oleg-nenashev commented Oct 25, 2016

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

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

Conversation

oleg-nenashev commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Oct 24, 2016

ghost commented Oct 24, 2016

Choose a reason for hiding this comment

rsandell commented Oct 24, 2016

oleg-nenashev commented Oct 24, 2016

rsandell commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented Oct 24, 2016

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Oct 25, 2016