Skip to content

Conversation

@platinumhamburg
Copy link
Contributor

Purpose

Linked issue: close #2177

Brief change log

Tests

API and Format

Documentation

@platinumhamburg platinumhamburg force-pushed the ratelimiter branch 2 times, most recently from a153b67 to 4aee7df Compare December 15, 2025 10:51
Copy link
Contributor

@swuferhong swuferhong left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions. I left some comments.

Copy link
Contributor

@swuferhong swuferhong left a comment

Choose a reason for hiding this comment

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

LGTM. Left one minor comment.

*
* -- Delete a configuration (reset to default)
* CALL sys.set_cluster_config('kv.shared-rate-limiter.bytes-per-sec', NULL);
* CALL sys.set_cluster_config('kv.shared-rate-limiter.bytes-per-sec', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe set_cluster_config and delete_cluster_config can be divided into different procedures?

} catch (Exception e) {
// Ignore cleanup errors to avoid masking test failures
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be moved into test cases related to set cluster configuration? It feels a bit odd to place them in afterEach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[KV] Add RocksDB rate limiter support for TabletServer

3 participants