Jitter throttling client retries so clients do not retry in lockstep

[1991santhu]

Description

The retry backoff in RedirectAwareRestClientRequestSender starts at a fixed 500ms, so every client that loses the throttling server computes an identical delay sequence and retries at the same instants.

These clients are already synchronised by construction. They fail together because the server they share became unreachable, so each retry round arrives at the throttling service as a burst, and the burst repeats as the sequence doubles. The service exists to limit request rate, which makes it a bad thing to hammer while it is recovering.

The fix randomises the initial delay:

private final ExponentialBackoff exponentialBackoff = ExponentialBackoff.builder()
    .maxDelay(RETRY_MAX_DELAY_MILLIS)
    .initialDelay(RETRY_MIN_INITIAL_DELAY_MILLIS
        + ThreadLocalRandom.current().nextLong(RETRY_INITIAL_DELAY_RANGE_MILLIS))
    .build();

Every subsequent delay is a multiple of the initial one, since ExponentialBackoff computes nextDelay = alpha * nextDelay + 1. So randomising the initial value decorrelates the whole sequence rather than just the first retry.

The random amount is added to the 500ms floor rather than centred on it, so a client never retries the throttling service sooner than the fixed delay already allowed.

Why the call site and not ExponentialBackoff

I looked at fixing this in the shared utility and decided against it, for three reasons.

maxWait there is a total budget rather than a per-delay cap. awaitNextRetry accumulates totalWait += nextDelay and stops once it passes maxWait. Extending each delay would consume that budget in fewer iterations, so callers that poll would get fewer attempts inside the same window. AsyncConverter1to1Test polls with awaitCondition().maxWait(100L), which is a 100ms budget, and would start flaking.

The utility is also dual-purpose, and the two uses want opposite things. As a poller (StreamModelTaskRunner, FlowCatalog) it wants tight predictable intervals inside a budget. As a contention backoff it wants spread. One knob cannot serve both.

And this repo already has an idiom for exactly this. MysqlMultiActiveLeaseArbiter randomises its own initial delay at the call site:

.initialDelay(MIN_INITIAL_DELAY_MILLIS + (long) (Math.random() * DELAY_FOR_RETRY_RANGE_MILLIS))

Worth noting that precedent is partial: it randomises only the initial delay, and subsequent delays still double deterministically. Same shape as this change.

The two Zookeeper callers turned up in my initial search but use Curator's ExponentialBackoffRetry, a different class that already randomises internally, so they are unaffected.

Tests

No new unit test. The change is a constructor argument on a private field inside CallbackDecorator, and asserting on a randomised delay would mean either exposing the backoff or asserting a distribution, neither of which seemed worth it for this. Happy to add one if a reviewer disagrees, and happy to take a [GOBBLIN-xxxx] ticket for the title if you would prefer one.

Verified it compiles on JDK 8, which is what the CI workflows pin:

./gradlew :gobblin-restli:gobblin-throttling-service:gobblin-throttling-service-client:compileJava
BUILD SUCCESSFUL in 32s
93 actionable tasks: 20 executed, 73 up-to-date