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

Timeout jlm server connection using elapsed time and nbr attempts #369

Merged
merged 4 commits into from
Nov 2, 2020

Conversation

lumpfish
Copy link
Contributor

attempts

Signed-off-by: Simon Rushton rushton@uk.ibm.com

attempts

Signed-off-by: Simon Rushton <rushton@uk.ibm.com>
@lumpfish
Copy link
Contributor Author

Fixes #370

@lumpfish lumpfish changed the title Timeout jlm server connection using elapsed time as well as number of Timeout jlm server connection using elapsed time and nbr attempts Oct 15, 2020
@karianna karianna added the bug label Oct 18, 2020
@karianna karianna added this to the October 2020 milestone Oct 18, 2020
if (attempts == 30) {
Message.logOut("Failed to connect to Monitored VM after 30 attempts - giving up. Connection Exception received is below:");
if (connectElapsed > connectTimeout || attempts == 30 ) {
Message.logOut("Failed to connect to Monitored VM after " + attempts + " attempts in " + (connectElapsed / 1000000000) + " seconds - giving up. Connection Exception received is below:");
Copy link
Contributor

Choose a reason for hiding this comment

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

magic number could be extracted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which 'magic number' are you referring to, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

The 30 and the 1000000000 could both be extracted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying. By 'extracted' you mean something like

maxAttempts = 30;
nanosecondsPerSecond = 1000L * 1000L * 1000L;

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like that yes, it's probably just personal programming style preference, but makes it easier to parameterize/override later.

Simon Rushton added 3 commits October 21, 2020 09:52
Signed-off-by: Simon Rushton <rushton@uk.ibm.com>
Signed-off-by: Simon Rushton <rushton@uk.ibm.com>
Signed-off-by: Simon Rushton <rushton@uk.ibm.com>
@lumpfish
Copy link
Contributor Author

@karianna - I've refactored the code to get rid of the duplicate code between getServerConnection() and getSecureServerConnection(). Please take another look.

New test run is here: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/4267.

When we're ready to merge, please can we "Squash and merge". No need to include my debugging and typo commits in the repository history.

Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

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

LGTM

@karianna
Copy link
Contributor

@lumpfish - I'm happy with it (thank you!) - save to merge during release day?

@lumpfish
Copy link
Contributor Author

lumpfish commented Nov 2, 2020

Merging now the Oct 2020 releases are finished.

@lumpfish lumpfish merged commit 9f9552c into adoptium:master Nov 2, 2020
@lumpfish lumpfish deleted the fix_jlmtest_connect_timeout branch November 2, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants