Skip to content

Adds retry mechanism to applyNullSafe method #204

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

Open
jlcontreras wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Adds retry mechanism to applyNullSafe method #204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd1bc3e
Added retry/backoff to GitHubRepositoryName.resolve method for robust…
Jan 4, 2019
7569a52
Moved retry mechanism to the applyNullSafe method
Jan 25, 2019
60385cb
Style fixes, updated findbugs-maven-plugin to v3.0.5 to work with mav…
Jan 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
<jenkins-test-harness.version>1.625.3</jenkins-test-harness.version>
<release.skipTests>false</release.skipTests>
<maven.javadoc.skip>true</maven.javadoc.skip>
<findbugs-maven-plugin.version>3.0.2</findbugs-maven-plugin.version>
<findbugs-maven-plugin.version>3.0.5</findbugs-maven-plugin.version>
Copy link

@stsukrov stsukrov Jan 31, 2019

Choose a reason for hiding this comment

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

Should we do this in a separate PR?
Or is it necessary for the fix?

Copy link
Author

@jlcontreras jlcontreras Jan 31, 2019

Choose a reason for hiding this comment

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

It is not necessary for this fix. I added it because I don't think it hurts, but it can be moved to a separate PR, or even not changed if there is any reason to keep it to 3.0.2.

Copy link

@stsukrov stsukrov Jan 31, 2019

Choose a reason for hiding this comment

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

Just about the atomicity of the commits.

<concurrency>1</concurrency>
<java.level>7</java.level>
<hpi-plugin.version>1.120</hpi-plugin.version>
Expand Down
27 changes: 22 additions & 5 deletions src/main/java/com/cloudbees/jenkins/GitHubRepositoryName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.io.IOException;
import java.net.UnknownHostException;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -64,6 +65,9 @@ public class GitHubRepositoryName {
Pattern.compile("ssh://(?:git@)?([^/]+)/([^/]+)/([^/]+)/?")
};

private static final int MAX_RETRIES = 3;
private static final int BACKOFF_MILLIS = 50;

/**
* Create {@link GitHubRepositoryName} from URL
*
Expand Down Expand Up @@ -223,12 +227,25 @@ private static Function<GitHub, GHRepository> toGHRepository(final GitHubReposit
return new NullSafeFunction<GitHub, GHRepository>() {
@Override
protected GHRepository applyNullSafe(@Nonnull GitHub gitHub) {
try {
return gitHub.getRepository(format("%s/%s", repoName.getUserName(), repoName.getRepositoryName()));
} catch (IOException e) {
LOGGER.warn("Failed to obtain repository {}", this, e);
return null;
int mtries = 0;
while (mtries < MAX_RETRIES) {
Copy link

@stsukrov stsukrov Jan 31, 2019

Choose a reason for hiding this comment

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

Is there some @annotation oriented retry mechanism in the project?
If so, can we use it?

Otherwise, can we look for some utility class/create one?

try {
return gitHub.getRepository(format("%s/%s", repoName.getUserName(),
repoName.getRepositoryName()));
} catch (UnknownHostException e) {
LOGGER.warn("Failed to resolve repository {}", this, e);
Copy link

@stsukrov stsukrov Jan 31, 2019

Choose a reason for hiding this comment

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

Was this code covered by a unit test?
Should we raise the bar?

mtries++;
try {
Thread.sleep(BACKOFF_MILLIS * mtries);
} catch (InterruptedException ex) {
LOGGER.error("{}", this, ex);
}
} catch (IOException e) {
LOGGER.warn("Failed to obtain repository {}", this, e);
return null;
}
}
return null;
}
};
}
Expand Down