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.

Sign up for GitHub

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

Jump to bottom

[FIX JENKINS-52739] set default values for retries and timeout settings #92

Merged
merged 6 commits into from
Jul 31, 2018
Merged

[FIX JENKINS-52739] set default values for retries and timeout settings #92

merged 6 commits into from
Jul 31, 2018

Conversation

kuisathaverat
Copy link
Contributor

@kuisathaverat kuisathaverat commented Jul 27, 2018 •
edited
Loading

JENKINS-52739

  • set DEFAULT_MAX_NUM_RETRIES = 10 tries
  • set DEFAULT_RETRY_WAIT_TIME = 15 seconds
  • set DEFAULT_LAUNCH_TIMEOUT_SECONDS = 10 (tries) * 15 (retry wait) + 60 seconds
  • Fix un treated exceptions types on timeout check

@kuisathaverat
Copy link
Contributor Author

relaunch CI

@kuisathaverat kuisathaverat reopened this Jul 27, 2018
kuisathaverat
kuisathaverat commented Jul 27, 2018
View reviewed changes
src/main/java/hudson/plugins/sshslaves/SSHLauncher.java Outdated
@@ -866,7 +881,7 @@ public Boolean call() throws InterruptedException {
Boolean res;
try {
res = results.get(0).get();
} catch (ExecutionException e) {
} catch (Exception e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CancelException is not treated, probably more, it breaks the retries logic, by catching the Exception the CancelException is treated.

olivergondza
olivergondza approved these changes Jul 27, 2018
View reviewed changes
Copy link
Member

@olivergondza olivergondza left a comment

Choose a reason for hiding this comment

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

Looks good to me. I doubt someone really needs this to wait forever. Integer range is big enough for a couple of decades worth of seconds for launch to timeout.

jeffret-b
jeffret-b reviewed Jul 27, 2018
View reviewed changes
Copy link

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

I've got a concern about catching Throwable but other than that it looks good.

src/main/java/hudson/plugins/sshslaves/SSHLauncher.java Outdated
@@ -866,8 +881,11 @@ public Boolean call() throws InterruptedException {
Boolean res;
try {
res = results.get(0).get();
} catch (ExecutionException e) {
} catch (Throwable e) {
Copy link

Choose a reason for hiding this comment

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

I'm a little concerned about catching Throwable here. It's not generally a good idea. Is this one of those few places where Throwable makes sense? Would it be better to narrow it down to not catch catastrophic failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, I will try to figure out which exceptions could be throw here, the main issue here is, if the exception is not catched, the retries start again from the max number of retries.

jvz
jvz suggested changes Jul 30, 2018
View reviewed changes
src/main/java/hudson/plugins/sshslaves/SSHLauncher.java
@@ -141,6 +142,9 @@
private static final List<String> RECOVERABLE_FAILURES = Arrays.asList(
"Connection refused", "Connection reset", "Connection timed out", "No route to host", "Premature connection close"
);
public static final Integer DEFAULT_MAX_NUM_RETRIES = 10;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense to use int here? The member fields can be nullable, but these constants certainly aren't, and autoboxing will be performed regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna check it because I have changed it due findbugs errors related to boxing-unboxing

Copy link
Contributor Author

@kuisathaverat kuisathaverat Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That's the reason why I set the constants as Integer

At SSHLauncher.java:[line 1439] BX_UNBOXING_IMMEDIATELY_REBOXED

    public Integer getMaxNumRetries() {
        return maxNumRetries == null || maxNumRetries < 0 ? DEFAULT_MAX_NUM_RETRIES : maxNumRetries;
    }

I can change it to but it is ugly to transform something can be and Integer, I preferred to avoid the transformation.

    public Integer getMaxNumRetries() {
        return maxNumRetries == null || maxNumRetries < 0 ? Integer.valueOf(DEFAULT_MAX_NUM_RETRIES) : maxNumRetries;
    }

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, no worries. This would require changing far more signatures clearly; not worth it.

src/main/java/hudson/plugins/sshslaves/SSHLauncher.java Outdated
@@ -1415,7 +1434,7 @@ private long getLaunchTimeoutMillis() {
* @return maxNumRetries
*/
public Integer getMaxNumRetries() {
Copy link
Member

Choose a reason for hiding this comment

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

Are you stuck with API compatibility here, or can you change these getters to use an int instead? Or add @NonNull to them at least? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

olivergondza
olivergondza reviewed Jul 31, 2018
View reviewed changes
src/main/java/hudson/plugins/sshslaves/SSHLauncher.java Outdated
res = Boolean.FALSE;
System.out.println(e.getMessage());
listener.getLogger().println("ECHO");
Copy link
Member

Choose a reason for hiding this comment

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

Looks like forgotten debugging statements...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, it is removed on latest commit ;)

@jvz
Copy link
Member

jvz commented Jul 31, 2018

You've got a typo in the title, otherwise all good.

@kuisathaverat kuisathaverat changed the title [JENKINS-52739] set default values for reties and timeout settings [JENKINS-52739] set default values for retries and timeout settings Jul 31, 2018
@kuisathaverat kuisathaverat merged commit f68802b into jenkinsci:master Jul 31, 2018
@kuisathaverat kuisathaverat changed the title [JENKINS-52739] set default values for retries and timeout settings [FIX JENKINS-52739] set default values for retries and timeout settings Aug 14, 2018
@kuisathaverat kuisathaverat deleted the JENKINS-52739 branch March 10, 2019 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olivergondza olivergondza olivergondza approved these changes

@jeffret-b jeffret-b jeffret-b left review comments

@jvz jvz jvz approved these changes

@dwnusbaum dwnusbaum Awaiting requested review from dwnusbaum

@varyvol varyvol Awaiting requested review from varyvol

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kuisathaverat @jvz @olivergondza @jeffret-b