-
Notifications
You must be signed in to change notification settings - Fork 646
feat(iceberg): enable iceberg compaction for iceberg table and add license check #22043
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
feat(iceberg): enable iceberg compaction for iceberg table and add license check #22043
Conversation
maybe related to #21970 https://buildkite.com/risingwavelabs/pull-request/builds/75287#01971404-461c-4485-9db9-d89c3d1b1905 snowflake sink is not gated by the paywall. |
All reactions
Sorry, something went wrong.
.is_ok() | ||
.to_string(), | ||
); | ||
|
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.
add compaction_interval_sec
, REST LGTM
Sorry, something went wrong.
All reactions
@@ -1714,6 +1714,14 @@ pub async fn create_iceberg_engine_table( | |||
|
|||
sink_with.insert("is_exactly_once".to_owned(), "true".to_owned()); | |||
|
|||
sink_with.insert( |
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.
check with_options.enable_compaction
Sorry, something went wrong.
All reactions
…table_and_add_license_check
@@ -1714,6 +1714,56 @@ pub async fn create_iceberg_engine_table( | |||
|
|||
sink_with.insert("is_exactly_once".to_owned(), "true".to_owned()); | |||
|
|||
if let Some(enable_compaction) = handler_args.with_options.get("enable_compaction") { |
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.
Here are two issues:
- This logic is implemented in a verbose way. We can encapsulate the logic for checking
enable_compaction
withinWithOptions
to reduce if branches. - When
enable_compaction == None
, we don't need to check the license and should setenable_compaction=false
.
Sorry, something went wrong.
All reactions
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.
For iceberg engine, we should try our best to manage the table, so we should enable compaction if possible. Users only need to tune the config when they encounter performance issues.
Sorry, something went wrong.
All reactions
-
👍 1 reaction
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.
Rest LGTM
Sorry, something went wrong.
All reactions
Li0k
xxchan
wenym1
xxhZs
Successfully merging this pull request may close these issues.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Checklist
Documentation
Release note