Skip to content

Bump bucket4j to 8.16 #3652

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

Draft
Rizziepit wants to merge 3 commits into
base: master
Choose a base branch
from
+139 −73
Draft

Bump bucket4j to 8.16 #3652

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de70d88
Bump bucket4j to 8.16
Rizziepit Feb 4, 2026
bf0756d
Fix misk-rate-limiting-bucket4j-dynamodb-v2
Rizziepit Feb 4, 2026
825f031
Bump bucket4j to 8.16 (#3653)
tgregory-block Feb 5, 2026
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
8 changes: 4 additions & 4 deletions gradle/libs.versions.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
awaitility = "4.3.0"
aws1 = "1.12.791"
aws2 = "2.37.2"
bucket4j = "8.8.0"
bucket4j = "8.16.1"
datadog = "1.53.0"
dependencyAnalysisPlugin = "3.0.4"
detekt = "1.23.8"
Expand Down Expand Up @@ -56,10 +56,10 @@ awsSqs = { module = "com.amazonaws:aws-java-sdk-sqs", version.ref = "aws1" }
bouncyCastlePgp = { module = "org.bouncycastle:bcpg-jdk18on", version = "1.82" }
bouncyCastlePkix = { module = "org.bouncycastle:bcpkix-jdk18on", version = "1.82" }
bouncyCastleProvider = { module = "org.bouncycastle:bcprov-jdk18on", version = "1.82" }
bucket4jCore = { module = "com.bucket4j:bucket4j-core", version.ref = "bucket4j" }
bucket4jCore = { module = "com.bucket4j:bucket4j_jdk17-core", version.ref = "bucket4j" }
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" }
Copy link
Collaborator

@tgregory-block tgregory-block Feb 4, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@Rizziepit Rizziepit Feb 4, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@tgregory-block tgregory-block Feb 5, 2026

Choose a reason for hiding this comment

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

@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

bucket4jRedis = { module = "com.bucket4j:bucket4j_jdk17-jedis", version.ref = "bucket4j" }
caffeine = { module = "com.github.ben-manes.caffeine:caffeine", version = "3.2.2" }
classGraph = { module = "io.github.classgraph:classgraph", version = "4.8.179" }
concurrencyLimitsCore = { module = "com.netflix.concurrency-limits:concurrency-limits-core", version = "0.5.3" }
Expand Down
6 changes: 4 additions & 2 deletions ...main/kotlin/misk/ratelimiting/bucket4j/dynamodb/v2/transaction/BaseDynamoDBTransaction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedExce

internal abstract class BaseDynamoDBTransaction(private val dynamoDB: DynamoDbClient, private val table: String) :
CompareAndSwapOperation {
override fun getStateData(): Optional<ByteArray> {
override fun getStateData(timeoutNanos: Optional<Long>): Optional<ByteArray> {
val attributes = mapOf(DEFAULT_KEY_NAME to getKeyAttributeValue())

// TODO: respect timeout
val result =
dynamoDB
.getItem {
Expand All @@ -36,7 +37,7 @@ internal abstract class BaseDynamoDBTransaction(private val dynamoDB: DynamoDbCl
return Optional.of(state.b().asByteArray())
}

override fun compareAndSwap(originalData: ByteArray?, newData: ByteArray, newState: RemoteBucketState?): Boolean {
override fun compareAndSwap(originalData: ByteArray?, newData: ByteArray, newState: RemoteBucketState?, timeoutNanos: Optional<Long>): Boolean {
val item =
mapOf(
DEFAULT_KEY_NAME to getKeyAttributeValue(),
Expand All @@ -46,6 +47,7 @@ internal abstract class BaseDynamoDBTransaction(private val dynamoDB: DynamoDbCl
mapOf(":expected" to AttributeValue.fromB(originalData?.toSdkBytes() ?: SdkBytes.fromUtf8String("")))
val names = mapOf("#st" to DEFAULT_STATE_NAME)

// TODO: respect timeouts
return try {
dynamoDB.putItem {
it.tableName(table)
Expand Down
19 changes: 11 additions & 8 deletions .../kotlin/misk/ratelimiting/bucket4j/dynamodb/v2/transaction/BaseDynamoDBTransactionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package misk.ratelimiting.bucket4j.dynamodb.v2.transaction

import jakarta.inject.Inject
import java.util.Optional
import misk.ratelimiting.bucket4j.dynamodb.v2.modules.DynamoDbLongTestModule.Companion.LONG_TABLE_NAME
import misk.ratelimiting.bucket4j.dynamodb.v2.modules.DynamoDbStringTestModule.Companion.STRING_TABLE_NAME
import misk.ratelimiting.bucket4j.dynamodb.v2.transaction.BaseDynamoDBTransaction.Companion.DEFAULT_KEY_NAME
Expand All @@ -18,7 +19,7 @@ abstract class BaseDynamoDBTransactionTest<K> {
@Test
fun `should return empty when no bucket exists`() {
val transaction = createTransaction(createRandomKey())
assertThat(transaction.stateData.isEmpty).isTrue()
assertThat(transaction.getStateData(Optional.empty<Long>()).isEmpty).isTrue()
}

@Test
Expand All @@ -28,7 +29,7 @@ abstract class BaseDynamoDBTransactionTest<K> {
saveState(key, null)

val transaction = createTransaction(key)
assertThat(transaction.stateData.isEmpty).isTrue()
assertThat(transaction.getStateData(Optional.empty<Long>()).isEmpty).isTrue()
}

@Test
Expand All @@ -39,7 +40,7 @@ abstract class BaseDynamoDBTransactionTest<K> {
saveState(key, state)

val transaction = createTransaction(key)
assertThrows<IllegalStateException> { transaction.stateData }
assertThrows<IllegalStateException> { transaction.getStateData(Optional.empty<Long>()) }
}

@Test
Expand All @@ -50,8 +51,8 @@ abstract class BaseDynamoDBTransactionTest<K> {
saveState(key, state)

val transaction = createTransaction(key)
assertThat(transaction.stateData.isPresent).isTrue()
assertThat(transaction.stateData.get()).isEqualTo(state.b().asByteArray())
assertThat(transaction.getStateData(Optional.empty<Long>()).isPresent).isTrue()
assertThat(transaction.getStateData(Optional.empty<Long>()).get()).isEqualTo(state.b().asByteArray())
}

@Test
Expand All @@ -60,7 +61,7 @@ abstract class BaseDynamoDBTransactionTest<K> {
val update = AttributeValue.fromB(SdkBytes.fromUtf8String("update"))
val transaction = createTransaction(key)

val result = transaction.compareAndSwap(null, update.b().asByteArray(), null)
val result = transaction.compareAndSwap(null, update.b().asByteArray(), null, Optional.empty<Long>())
val state = getState(key)

assertThat(result).isTrue()
Expand All @@ -74,7 +75,8 @@ abstract class BaseDynamoDBTransactionTest<K> {
val update = AttributeValue.fromB(SdkBytes.fromUtf8String("update"))
val transaction = createTransaction(key)

val result = transaction.compareAndSwap(original.b().asByteArray(), update.b().asByteArray(), null)
val result =
transaction.compareAndSwap(original.b().asByteArray(), update.b().asByteArray(), null, Optional.empty<Long>())
val state = getState(key)

assertThat(result).isTrue()
Expand All @@ -91,7 +93,8 @@ abstract class BaseDynamoDBTransactionTest<K> {
saveState(key, initial)
val transaction = createTransaction(key)

val result = transaction.compareAndSwap(original.b().asByteArray(), update.b().asByteArray(), null)
val result =
transaction.compareAndSwap(original.b().asByteArray(), update.b().asByteArray(), null, Optional.empty<Long>())
val state = getState(key)

assertThat(result).isFalse()
Expand Down
4 changes: 3 additions & 1 deletion ...-redis/src/main/kotlin/misk/ratelimiting/bucket4j/redis/RedisBucket4jRateLimiterModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ constructor(
.withClientSideConfig(
// Use Clock instead of calling System.currentTimeMillis() for refill determination
// Equivalent logic at runtime, but lets us mock the refill times in integration tests
ClientSideConfig.getDefault().withClientClock(ClockTimeMeter(clock))
ClientSideConfig.getDefault()
.withClientClock(ClockTimeMeter(clock))
.withMaxRetries(5)
Copy link
Collaborator Author

@Rizziepit Rizziepit Feb 4, 2026

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@yyuan-squareup yyuan-squareup Feb 4, 2026

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@Rizziepit Rizziepit Feb 4, 2026

Choose a reason for hiding this comment

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

Looks like @tgregory-block added maxRetries in his PR. Let's continue to convo there #3653

Copy link
Collaborator Author

@Rizziepit Rizziepit Feb 4, 2026

Choose a reason for hiding this comment

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

#3653 (comment)

)
.withExpirationStrategy(
// Set Redis TTLs to the bucket refill period + additionalTtl
Expand Down
Loading