-
Notifications
You must be signed in to change notification settings - Fork 587
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(sink): set default_sink_decouple = true for all sink #18182
Conversation
57093f1
to
785e166
Compare
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.
For sinks that can configure commit_checkpoint_interval
, will increasing commit_checkpoint_interval
from 1 to 10 cause more memory pressure on the compute node? In other words, is the buffering before commit happen in RW CN's memory or in external sink?
@wenym1 PTAL as well.
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
@@ -17,6 +17,7 @@ CREATE SINK s6 AS select mv6.v1 as v1, mv6.v2 as v2, mv6.v3 as v3, mv6.v4 as v4, | |||
clickhouse.password = '', | |||
clickhouse.database = 'default', | |||
clickhouse.table='demo_test', | |||
commit_checkpoint_interval = 1, |
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.
Is setting this field just to implicitly enable sink decouple, or it becomes required to set this field after this PR?
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.
Because sink_decouple = false will report an error when commit_checkpoint_interval >10;
And in ci , we need sink_decouple = false to ensure that flush can flush data to sink.
So we set commit_checkpoint_interval = 1,
After this PR, if it is not set, a default value of 10 will be given
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 and Thanks!
By the way, there are several open PRs listed in #17095 (comment), cc. reviewer @wenym1
/// Commit every n(>0) checkpoints, default is 10. | ||
#[serde(default = "default_commit_checkpoint_interval")] | ||
#[serde_as(as = "DisplayFromStr")] | ||
pub commit_checkpoint_interval: u64, |
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.
Regarding of the change to default value, don't forget to update the doc as well. :)
Why not change the sink test as well? This PR changes the default behavior of all sinks to decouple. We should make the test compatible with this change. BTW, what if we
|
su
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
#17095
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
after this pr,
all sink sink_decouple default is true
And starrocks clickhouse iceberg deltalake 's default commit_checkpoint_interval is 1