8000 feat(iceberg): enable iceberg compaction for iceberg table and add license check by chenzl25 · Pull Request #22043 · risingwavelabs/risingwave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Conversation

chenzl25
Copy link
Contributor
@chenzl25 chenzl25 commented May 28, 2025

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

  • Enable iceberg compaction for iceberg table and add license check
  • Set default compaction interval secs to 3600s.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

@tabVersion
Copy link
Contributor

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.

.is_ok()
.to_string(),
);

Copy link
Contributor

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

@@ -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(
Copy link
Contributor

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

@chenzl25 chenzl25 requested a review from Li0k May 29, 2025 06:13
@@ -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") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here are two issues:

  1. This logic is implemented in a verbose way. We can encapsulate the logic for checking enable_compaction within WithOptions to reduce if branches.
  2. When enable_compaction == None, we don't need to check the license and should set enable_compaction=false.

Copy link
Contributor Author

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.

Copy link
Contributor
@Li0k Li0k left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@Li0k Li0k mentioned this pull request May 29, 2025
8 tasks
@chenzl25 chenzl25 added this pull request to the merge queue May 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2025
@chenzl25 chenzl25 added this pull request to the merge queue May 29, 2025
Merged via the queue into main with commit d8a6c8a May 29, 2025
29 of 30 checks passed
@chenzl25 chenzl25 deleted the dylan/enable_iceberg_compaction_for_iceberg_table_and_add_license_check branch May 29, 2025 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0