-
Notifications
You must be signed in to change notification settings - Fork 737
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
Add sanity caching and retry controls #1744
Changes from 2 commits
60a3fc6
744911f
df7b0d3
7cc9552
2a94473
21aa01e
f01ddc8
7a73735
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
import java.time.format.DateTimeFormatter; | ||
import java.time.temporal.ChronoUnit; | ||
import java.util.*; | ||
import java.util.concurrent.ThreadLocalRandom; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import java.util.logging.Logger; | ||
|
||
|
@@ -45,11 +46,22 @@ | |
class GitHubClient { | ||
|
||
/** The Constant CONNECTION_ERROR_RETRIES. */ | ||
static final int CONNECTION_ERROR_RETRIES = 2; | ||
/** | ||
* If timeout issues let's retry after milliseconds. | ||
*/ | ||
static final int retryTimeoutMillis = 100; | ||
private static final int DEFAULT_CONNECTION_ERROR_RETRIES = 2; | ||
|
||
/** The Constant DEFAULT_MINIMUM_RETRY_TIMEOUT_MILLIS. */ | ||
private static final int DEFAULT_MINIMUM_RETRY_MILLIS = 100; | ||
|
||
/** The Constant DEFAULT_MAXIMUM_RETRY_TIMEOUT_MILLIS. */ | ||
private static final int DEFAULT_MAXIMUM_RETRY_MILLIS = DEFAULT_MINIMUM_RETRY_MILLIS; | ||
|
||
// WARNING: These are unsupported environment variables. The GitHubClient class is internal and may change at any | ||
// time. | ||
private static final int retryCount = Math.max(DEFAULT_CONNECTION_ERROR_RETRIES, | ||
Integer.getInteger(GitHubClient.class.getName() + ".retryLimit", DEFAULT_CONNECTION_ERROR_RETRIES)); | ||
private static final int minRetryInterval = Math.max(DEFAULT_MINIMUM_RETRY_MILLIS, | ||
Integer.getInteger(GitHubClient.class.getName() + ".minRetryInterval", DEFAULT_MINIMUM_RETRY_MILLIS)); | ||
private static final int maxRetryInterval = Math.max(DEFAULT_MAXIMUM_RETRY_MILLIS, | ||
Integer.getInteger(GitHubClient.class.getName() + ".maxRetryInterval", DEFAULT_MAXIMUM_RETRY_MILLIS)); | ||
|
||
// Cache of myself object. | ||
private final String apiUrl; | ||
|
@@ -64,6 +76,9 @@ | |
@Nonnull | ||
private final AtomicReference<GHRateLimit> rateLimit = new AtomicReference<>(GHRateLimit.DEFAULT); | ||
|
||
@Nonnull | ||
private final GitHubSanityCachedValue<GHRateLimit> sanityCachedRateLimit = new GitHubSanityCachedValue<>(); | ||
|
||
private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName()); | ||
|
||
private static final ObjectMapper MAPPER = new ObjectMapper(); | ||
|
@@ -264,15 +279,19 @@ | |
GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException { | ||
GHRateLimit result; | ||
try { | ||
GitHubRequest request = GitHubRequest.newBuilder() | ||
final GitHubRequest request = GitHubRequest.newBuilder() | ||
.rateLimit(RateLimitTarget.NONE) | ||
.withApiUrl(getApiUrl()) | ||
.withUrlPath("/rate_limit") | ||
.build(); | ||
result = this | ||
.sendRequest(request, | ||
(connectorResponse) -> GitHubResponse.parseBody(connectorResponse, JsonRateLimit.class)) | ||
.body().resources; | ||
// Even when explicitly asking for rate limit, restrict to sane query frequency | ||
// return cached value if available | ||
result = this.sanityCachedRateLimit | ||
.get(() -> this | ||
.sendRequest(request, | ||
(connectorResponse) -> GitHubResponse.parseBody(connectorResponse, | ||
JsonRateLimit.class)) | ||
.body().resources); | ||
} catch (FileNotFoundException e) { | ||
// For some versions of GitHub Enterprise, the rate_limit endpoint returns a 404. | ||
LOGGER.log(FINE, "/rate_limit returned 404 Not Found."); | ||
|
@@ -421,7 +440,7 @@ | |
@Nonnull | ||
public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull BodyHandler<T> handler) | ||
throws IOException { | ||
int retries = CONNECTION_ERROR_RETRIES; | ||
int retries = retryCount; | ||
GitHubConnectorRequest connectorRequest = prepareConnectorRequest(request); | ||
do { | ||
GitHubConnectorResponse connectorResponse = null; | ||
|
@@ -632,11 +651,15 @@ | |
|
||
private static void logRetryConnectionError(IOException e, URL url, int retries) throws IOException { | ||
// There are a range of connection errors where we want to wait a moment and just automatically retry | ||
long sleepTime = minRetryInterval; | ||
if (maxRetryInterval > minRetryInterval) { | ||
sleepTime = ThreadLocalRandom.current().nextLong(minRetryInterval, maxRetryInterval); | ||
} | ||
LOGGER.log(INFO, | ||
e.getMessage() + " while connecting to " + url + ". Sleeping " + GitHubClient.retryTimeoutMillis | ||
e.getMessage() + " while connecting to " + url + ". Sleeping " + sleepTime | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recommend creating a trace ID here for debug logging. That way an admin can search debug logs and find related logs for a single retry sequence. Here's an example
When I enable debug logging for a class in the mentioned class it is so active in parallel that all of the logs come in out of order. Because of that, using the For exmaple, I default to retries of 30 in my class but I found in practice with GitHub it could retry up to 28 times. Because that was so close to the max retry limit I increased the retry limit to 60 in my particular setup. I also set the minimum time between retries to be 1000ms and the maximum to be 3000ms. I've found GitHub requiring me to retry up to 1 minute in these scenarios because of secondary API limits. The new secondary API limits are very aggressive at the moment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize the logging mechanisms have to change a little bit with my feedback; they're not as straightforward as making the change just in this area of code. |
||
+ " milliseconds before retrying... ; will try " + retries + " more time(s)"); | ||
try { | ||
Thread.sleep(GitHubClient.retryTimeoutMillis); | ||
Thread.sleep(sleepTime); | ||
} catch (InterruptedException ie) { | ||
throw (IOException) new InterruptedIOException().initCause(e); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
package org.kohsuke.github; | ||
|
||
import org.kohsuke.github.function.SupplierThrows; | ||
|
||
import java.time.Instant; | ||
|
||
/** | ||
* GitHubSanityCachedValue limits queries for a particular value to once per second. | ||
*/ | ||
class GitHubSanityCachedValue<T> { | ||
|
||
private final Object lock = new Object(); | ||
private long lastQueriedAtEpochSeconds = 0; | ||
private T lastResult = null; | ||
|
||
/** | ||
* Gets the value from the cache or calls the supplier if the cache is empty or out of date. | ||
* | ||
* @param query | ||
* a supplier the returns an updated value. Only called if the cache is empty or out of date. | ||
* @return the value from the cache or the value returned from the supplier. | ||
* @throws E | ||
* the exception thrown by the supplier if it fails. | ||
*/ | ||
<E extends Throwable> T get(SupplierThrows<T, E> query) throws E { | ||
synchronized (lock) { | ||
if (Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the delay of "1 second" should be configurable as well ? |
||
lastResult = query.get(); | ||
lastQueriedAtEpochSeconds = Instant.now().getEpochSecond(); | ||
} | ||
} | ||
return lastResult; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package org.kohsuke.github.function; | ||
|
||
/** | ||
* A functional interface, equivalent to {@link java.util.function.Supplier} but that allows throwing {@link Throwable} | ||
* | ||
* @param <T> | ||
* the type of output | ||
* @param <E> | ||
* the type of error | ||
*/ | ||
@FunctionalInterface | ||
public interface SupplierThrows<T, E extends Throwable> { | ||
/** | ||
* Get a value. | ||
* | ||
* @return the | ||
* @throws E | ||
* the exception that may be thrown | ||
*/ | ||
T get() throws E; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest setting
minRetryInterval
andmaxRetryInterval
viaInteger.getInteger
property here.