Conversation
| ClientSideConfig.getDefault().withClientClock(ClockTimeMeter(clock)) | ||
| ClientSideConfig.getDefault() | ||
| .withClientClock(ClockTimeMeter(clock)) | ||
| .withMaxRetries(5) |
There was a problem hiding this comment.
This is why we want to bump the bucket4j version. Previous versions of bucket4j have infinite retries. We suspect the infinite retries create, or contribute to, a thundering herd. This has overwhelmed our Redis cluster on multiple occasions
There was a problem hiding this comment.
I think we should allow the user to pass in a maxRetries but with a sensible default, instead of hardcode to 5. Also RetryStrategy seems to be good to have too
There was a problem hiding this comment.
Looks like @tgregory-block added maxRetries in his PR. Let's continue to convo there #3653
|
The build is failing because |
| bucket4jDynamoDbV1 = { module = "com.bucket4j:bucket4j-dynamodb-sdk-v1", version = "8.6.0" } | ||
| bucket4jMySQL = { module = "com.bucket4j:bucket4j-mysql", version.ref = "bucket4j" } | ||
| bucket4jRedis = { module = "com.bucket4j:bucket4j-redis", version.ref = "bucket4j" } | ||
| bucket4jMySQL = { module = "com.bucket4j:bucket4j_jdk17-mysql", version.ref = "bucket4j" } |
There was a problem hiding this comment.
As an aside, I have noticed a few libraries we depend on that are compiled against JDK 17, but misk itself is JDK 11. Is this possible only because our container runtime is ultimately Java 21?
There was a problem hiding this comment.
Oh I didn't know Misk is jdk 11. I went with jdk 17 only because the relocation notices here point to the jdk17 packages. I don't know what the implications are of compiling against different JDK versions.
There was a problem hiding this comment.
@adrw any thoughts here? I know it wouldn't be the first library in misk to do this, and I think the retry feature we want is only available in 8.16.x which seems to be Java 17 only
I think we have two options:
The background here is that the bucket4j maintainer dropped Dynamo support entirely because he resides in Russia and lost the ability to download the sources/containers he needed to test the dynamo module |
Is a 3rd option possible where we use 2 bucket4j-core versions in Misk? It might be better than no. 1 where services will end up with both the Dynamo AWS SDK v1 and v2 APIs available in their project (I think?). That might be pretty common already though. |
We could do that if we like shaded the earlier version, though I'd prefer to just implement the dynamo v1 bucket proxy ourselves given that it's just copy pasta from the v2 implementation we have +- some minor API differences. On option 1, I figured a mix would be okay given that v1 is already EoL and folks should be moving anyway |
* Implement support for new retry parameters * Add config mutator functions to bucket4j modules
3 participants
Description
Testing Strategy
Checklist
Thank you for contributing to Misk! 🎉