Support shared RocksDB rate limiter in Fluss #2178
Conversation
platinumhamburg
force-pushed
the
ratelimiter
branch
2 times, most recently
from
a153b67 to
4aee7df
Compare
...mon/src/main/java/org/apache/fluss/flink/procedure/SetSharedRocksDBRateLimiterProcedure.java
Outdated
Show resolved
Hide resolved
swuferhong
left a comment
swuferhong
left a comment
There was a problem hiding this comment.
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
platinumhamburg
force-pushed
the
ratelimiter
branch
2 times, most recently
from
6382ed3 to
df9a263
Compare
swuferhong
left a comment
swuferhong
left a comment
There was a problem hiding this comment.
LGTM. Left one minor comment.
...s-flink-common/src/main/java/org/apache/fluss/flink/procedure/SetClusterConfigProcedure.java
Outdated
Show resolved
Hide resolved
.../fluss-flink-common/src/test/java/org/apache/fluss/flink/procedure/FlinkProcedureITCase.java
Outdated
Show resolved
Hide resolved
wuchong
left a comment
wuchong
left a comment
There was a problem hiding this comment.
Thanks @platinumhamburg for the contribution. The code quality is very high. The new introduced ConfigValidator and set_cluster_config procedures look very nice to me.
I only left some minor comments below.
fluss-server/src/main/java/org/apache/fluss/server/kv/KvTablet.java
Outdated
Show resolved
Hide resolved
...s-flink-common/src/main/java/org/apache/fluss/flink/procedure/GetClusterConfigProcedure.java
Outdated
Show resolved
Hide resolved
...s-flink-common/src/main/java/org/apache/fluss/flink/procedure/GetClusterConfigProcedure.java
Outdated
Show resolved
Hide resolved
...s-flink-common/src/main/java/org/apache/fluss/flink/procedure/GetClusterConfigProcedure.java
Show resolved
Hide resolved
fluss-server/src/main/java/org/apache/fluss/server/kv/KvConfigValidator.java
Outdated
Show resolved
Hide resolved
platinumhamburg
force-pushed
the
ratelimiter
branch
from
1620e4a to
3018990
Compare
platinumhamburg
force-pushed
the
ratelimiter
branch
from
3018990 to
0b6f1c8
Compare
|
I've updated this PR and resolved all comments above, thanks for review again. |
wuchong
left a comment
wuchong
left a comment
There was a problem hiding this comment.
Thanks @platinumhamburg , this PR is already in a good shape. I only left some minor comments.
fluss-server/src/main/java/org/apache/fluss/server/DynamicServerConfig.java
Show resolved
Hide resolved
fluss-server/src/main/java/org/apache/fluss/server/DynamicServerConfig.java
Show resolved
Hide resolved
fluss-server/src/main/java/org/apache/fluss/server/DynamicServerConfig.java
Outdated
Show resolved
Hide resolved
fluss-server/src/test/java/org/apache/fluss/server/kv/KvTabletTest.java
Outdated
Show resolved
Hide resolved
fluss-server/src/main/java/org/apache/fluss/server/kv/rocksdb/RocksDBResourceContainer.java
Outdated
Show resolved
Hide resolved
Thanks for your detailed review, all comments resolved now. |
wuchong
left a comment
wuchong
left a comment
There was a problem hiding this comment.
@platinumhamburg Sorry for the confusion! What I meant by “place the Procedures documentation as a separate page under the DDL page” was actually to put “Procedures” at the same navigation level as “DDL”—not nested inside it—just like Iceberg does: https://iceberg.apache.org/docs/nightly/spark-procedures/.
I’ve updated the structure accordingly and reverted the changes to ddl.md.
I’ll merge this PR once CI passes.
4 participants
Purpose
Linked issue: close #2177
Brief change log
Tests
API and Format
Documentation