[FIX JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds #2645

alvarolobato · 2016-11-24T14:23:49Z

This PR supersedes #1687 give that the later has been stalled for more than a year.

The objective is basically the same:

Define ping interval in seconds and be able to customize it with a system property
Define timeout in seconds and be able to customize it with a system property

This will be updated after merge: https://wiki.jenkins-ci.org/display/JENKINS/Features+controlled+by+system+properties
@reviewbybees

ghost · 2016-11-24T14:28:36Z

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.

amuniz · 2016-11-24T16:55:26Z

core/src/test/java/hudson/slaves/ChannelPingerTest.java

+
+        verify(mockChannel).call(new ChannelPinger.SetUpRemotePing(42, 73));
+        verifyStatic();
+        ChannelPinger.setUpPingForChannel(mockChannel, 42, 73, true);


Are this calls at the end of all test methods testing something specific?

Yes,

verifyStatic(); ChannelPinger.setUpPingForChannel(mockChannel, 42, 73, true);

verifies that the method ChannelPinger.setUpPingForChannel(mockChannel, 42, 73, true); is called with those parameter in that order.

I know, really odd syntax.

amuniz · 2016-11-24T16:56:13Z

pom.xml

+      <dependency>
+        <groupId>com.google.guava</groupId>
+        <artifactId>guava-testlib</artifactId>
+        <version>${guavaVersion}</version>


<scope>test</scope>?

Ok, I see it's the child module.

right, that's DependencyManagement

amuniz · 2016-11-24T16:58:05Z

🐝 and 👍

oleg-nenashev · 2016-11-24T17:29:37Z

core/src/main/java/hudson/slaves/ChannelPinger.java


    public ChannelPinger() {
-        pingInterval = SystemProperties.getInteger(SYS_PROPERTY_NAME, DEFAULT_PING_INTERVAL_MIN);


🐛 Why do you get rid of SystemProperties

It was a mistake, changing it.

oleg-nenashev · 2016-11-24T17:31:22Z

core/src/main/java/hudson/slaves/ChannelPinger.java

-        private int pingInterval;
-        public SetUpRemotePing(int pingInterval) {
-            this.pingInterval = pingInterval;
+        private transient int pingInterval;


Should be deprecated?

This is a callable, why not just remove it?

To be compatible in cases were we have the old version of the class on the other side. I had it removed at first but then decided to go the safe way. Are we sure that this situation cannot happen?, I so, I will remove it.

oleg-nenashev

🐛 for not using SystemProperties. If there is a point in it, it needs to be justified

batmat

🐝 though I think the code at the beginning could be made leaner

batmat · 2016-11-24T20:54:11Z

core/src/main/java/hudson/slaves/ChannelPinger.java

+            } catch (NumberFormatException e) {
+                LOGGER.warning("Ignoring invalid " + SYS_PROPERTY_NAME_TIMEOUT_SECONDS + "=" + timeout);
+            }
+        }


I think this code should use SystemProperties#getInteger() isn't it?

And if you want to log warnings, then maybe we just want to create a third overloaded method to specify the desired Level?

Yes I'm going to fix that, it really needs to use SystemProperties instead of System.getProperty to be able to get the properties from the ServletContextbut I also have to take into account the deprecated property, so is not going to be so lean.

Are we sure that all code paths that lead to evaluating the system property are assured to be on the master's JVM... if not, then you actually had this correct... if on master only, there's a marker interface you are missing

@stephenc yes this is always on the master, the only part used on the agents is SetUpRemotePing callable. Which marker interface? Isn't it better to just use SystemProperties

Yes. Use SystemProperties when you know the value will only ever be queried from the master

batmat · 2016-11-24T21:22:52Z

Just filed #2646 for possible future usage here and elsewhere.

daniel-beck · 2016-11-25T09:23:22Z

core/src/main/java/hudson/slaves/ChannelPinger.java

            return null;
        }
+
+        protected Object readResolve() {


Is this really necessary?

alvarolobato · 2016-11-25T16:37:06Z

@oleg-nenashev I've changed it to use SystemProperties care to review please?

daniel-beck · 2016-11-26T21:48:48Z

👎 This appears to copy nontrivial amounts of code from #1687 without any attribution. If you're reusing parts of that PR (such as the practically identical tests), you should have e.g. the first commit be just (parts of) the other PR with @deadmoose as commit author.

alvarolobato · 2016-11-28T07:42:31Z

@daniel-beck I didn't want to take authoring just unblock this, I'll take #1687 as base.

alvarolobato · 2016-11-28T08:45:41Z

@daniel-beck I've added #1687 commits to this PR.

daniel-beck · 2016-11-28T23:58:57Z

This should just be rebased onto @deadmoose's commit, or a commit declaring deadmoose as author that includes the parts you're incorporating into your PR. There's still commits with questionable authorship in this PR's history.

* Allows customization in seconds, not minutes * Allows customization of the ping timeout (before, you could set a custom interval, but the timeout would always be PingThread's 4 minute default) This also drops the serialVersionUID from ChannelPinger.SetUpRemotePing; without one provided, the JVM will generate one on demand which is sufficient for the purposes here since these are never persisted and master & slave run the same compiled code. (And it demonstrably works since countless other MasterToSlaveCallables fail to specify their own custom IDs)

…g interval and ping timeout in seconds

alvarolobato · 2016-11-29T10:28:46Z

@daniel-beck, yet another try... I hope all the autoring is clear this time

oleg-nenashev

LGTM 🐝

alvarolobato · 2016-12-12T13:25:54Z

@reviewbybees done

batmat

👍 🐝

oleg-nenashev · 2016-12-14T09:39:11Z

@alvarolobato could you please update the Wiki page?

alvarolobato · 2016-12-15T08:23:08Z

@oleg-nenashev done

Noting #2668, #2664, #2663, #2662, #2661, #2667, #2666, #2652, #2643, #2591, #2658, #2645, #2660, #2603

alvarolobato changed the title ~~[JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds~~ [FIX JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds Nov 24, 2016

amuniz reviewed Nov 24, 2016

View reviewed changes

oleg-nenashev reviewed Nov 24, 2016

View reviewed changes

oleg-nenashev requested changes Nov 24, 2016

View reviewed changes

batmat approved these changes Nov 24, 2016

View reviewed changes

daniel-beck reviewed Nov 25, 2016

View reviewed changes

core/src/main/java/hudson/slaves/ChannelPinger.java

return null;

}

protected Object readResolve() {

Copy link

Member

daniel-beck Nov 25, 2016

Choose a reason for hiding this comment

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

Is this really necessary?

batmat mentioned this pull request Nov 25, 2016

Be able to specify the log Level when system property is not found #2646

Merged

oleg-nenashev added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Nov 27, 2016

alvarolobato closed this Nov 28, 2016

alvarolobato reopened this Nov 28, 2016

batmat removed the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Nov 28, 2016

batmat mentioned this pull request Nov 28, 2016

[FIXES JENKINS-28245] Allow finer-grained tuning of ChannelPinger. #1687

Closed

deadmoose and others added 2 commits November 29, 2016 10:28

[JENKINS-28245] - Finish deadmoose's work to allow defining agent pin…

6dea3c3

…g interval and ping timeout in seconds

alvarolobato closed this Nov 29, 2016

alvarolobato force-pushed the JENKINS-28245 branch from 7f11459 to a05d942 Compare November 29, 2016 10:18

alvarolobato reopened this Nov 29, 2016

oleg-nenashev approved these changes Nov 30, 2016

View reviewed changes

daniel-beck added the needs-more-reviews Complex change, which would benefit from more eyes label Nov 30, 2016

batmat approved these changes Dec 12, 2016

View reviewed changes

oleg-nenashev merged commit 1868c84 into jenkinsci:master Dec 14, 2016

oleg-nenashev removed the needs-more-reviews Complex change, which would benefit from more eyes label Dec 15, 2016

oleg-nenashev added a commit that referenced this pull request Dec 16, 2016

Changelog: updates towards 2.37

1c1e9d5

Noting #2668, #2664, #2663, #2662, #2661, #2667, #2666, #2652, #2643, #2591, #2658, #2645, #2660, #2603

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds #2645

[FIX JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds #2645

alvarolobato commented Nov 24, 2016

ghost commented Nov 24, 2016

amuniz Nov 24, 2016

alvarolobato Nov 24, 2016

amuniz Nov 24, 2016

amuniz Nov 24, 2016

alvarolobato Nov 24, 2016

amuniz commented Nov 24, 2016

oleg-nenashev Nov 24, 2016

alvarolobato Nov 24, 2016

oleg-nenashev Dec 14, 2016

oleg-nenashev Nov 24, 2016

alvarolobato Nov 24, 2016

daniel-beck Nov 25, 2016

alvarolobato Nov 25, 2016

oleg-nenashev left a comment

batmat left a comment

batmat Nov 24, 2016

alvarolobato Nov 24, 2016

stephenc Nov 24, 2016

alvarolobato Nov 25, 2016

stephenc Nov 25, 2016

batmat commented Nov 24, 2016

daniel-beck Nov 25, 2016

alvarolobato commented Nov 25, 2016

daniel-beck commented Nov 26, 2016

alvarolobato commented Nov 28, 2016 •

edited

Loading

alvarolobato commented Nov 28, 2016

daniel-beck commented Nov 28, 2016

alvarolobato commented Nov 29, 2016

oleg-nenashev left a comment

alvarolobato commented Dec 12, 2016

batmat left a comment

oleg-nenashev commented Dec 14, 2016

alvarolobato commented Dec 15, 2016


		public ChannelPinger() {
		pingInterval = SystemProperties.getInteger(SYS_PROPERTY_NAME, DEFAULT_PING_INTERVAL_MIN);

[FIX JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds #2645

[FIX JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds #2645

Conversation

alvarolobato commented Nov 24, 2016

ghost commented Nov 24, 2016

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

Choose a reason for hiding this comment

amuniz commented Nov 24, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

batmat 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batmat commented Nov 24, 2016

Choose a reason for hiding this comment

alvarolobato commented Nov 25, 2016

daniel-beck commented Nov 26, 2016

alvarolobato commented Nov 28, 2016 • edited Loading

alvarolobato commented Nov 28, 2016

daniel-beck commented Nov 28, 2016

alvarolobato commented Nov 29, 2016

oleg-nenashev left a comment

Choose a reason for hiding this comment

alvarolobato commented Dec 12, 2016

batmat left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Dec 14, 2016

alvarolobato commented Dec 15, 2016

alvarolobato commented Nov 28, 2016 •

edited

Loading