-
Notifications
You must be signed in to change notification settings - Fork 448
Support shared RocksDB rate limiter in Fluss #2178
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
base: main
Are you sure you want to change the base?
Conversation
a153b67 to
4aee7df
Compare
...mon/src/main/java/org/apache/fluss/flink/procedure/SetSharedRocksDBRateLimiterProcedure.java
Outdated
Show resolved
Hide resolved
swuferhong
left a comment
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.
Thanks for your contributions. I left some comments.
...mon/src/main/java/org/apache/fluss/flink/procedure/SetSharedRocksDBRateLimiterProcedure.java
Outdated
Show resolved
Hide resolved
...mon/src/main/java/org/apache/fluss/flink/procedure/GetSharedRocksDBRateLimiterProcedure.java
Outdated
Show resolved
Hide resolved
4aee7df to
d44ea65
Compare
6382ed3 to
df9a263
Compare
swuferhong
left a comment
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.
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', ''); |
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.
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 | ||
| } | ||
| } |
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.
Can these be moved into test cases related to set cluster configuration? It feels a bit odd to place them in afterEach?
Purpose
Linked issue: close #2177
Brief change log
Tests
API and Format
Documentation