Skip to content

feat(sink): support sink rate limit for external sink - #19660

Merged
MrCroxx merged 13 commits into
mainfrom
patrick/sink-rate-limit.pr
Dec 23, 2024
Merged

MrCroxx merged 13 commits into
mainfrom
patrick/sink-rate-limit.pr

Conversation

@hzxa21

@hzxa21 hzxa21 commented Dec 3, 2024 •

Copy link
Copy Markdown
Collaborator

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

What's changed and what's your intention?

Closes #16253

Support SET SINK_RATE_LIMIT and ALTER SINK ... SET SINK_RATE_LIMIT for external sink. Sink into table is not supported.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

Support session config SET SINK_RATE_LIMIT to ... and ALTER SINK ... SET SINK_RATE_LIMIT to ... for external sink. Sink into table is not supported. See example in rate_limit.slt

@hzxa21
hzxa21 requested a review from a team as a code owner December 3, 2024 17:29
@hzxa21
hzxa21 requested a review from lmatz December 3, 2024 17:29
@graphite-app
graphite-app Bot requested a review from a team December 4, 2024 14:11
@lmatz lmatz added the user-facing-changes Contains changes that are visible to users label Dec 5, 2024
@github-actions

github-actions Bot commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

Hi, there.

📝 Telemetry Reminder:
If you're implementing this feature, please consider adding telemetry metrics to track its usage. This helps us understand how the feature is being used and improve it further.
You can find the function report_event of telemetry reporting in the following files. Feel free to ask questions if you need any guidance!

  • src/frontend/src/telemetry.rs
  • src/meta/src/telemetry.rs
  • src/stream/src/telemetry.rs
  • src/storage/compactor/src/telemetry.rs
    Or calling report_event_common (src/common/telemetry_event/src/lib.rs) as if finding it hard to implement.
    ✨ Thank you for your contribution to RisingWave! ✨

This is an automated comment created by the peaceiris/actions-label-commenter. Responding to the bot or mentioning it won't have any effect.

@tabVersion

Copy link
Copy Markdown
Contributor

quick question: if the sink rate limit makes more data buffered in LogStore?

@hzxa21

hzxa21 commented Dec 6, 2024

Copy link
Copy Markdown
Collaborator Author

quick question: if the sink rate limit makes more data buffered in LogStore?

If sink decoupled is on, yes. If sink decoupled is off, it will cause backpressure.

@hzxa21
hzxa21 force-pushed the patrick/sink-rate-limit.pr branch 2 times, most recently from 66b212a to 908c638 Compare December 16, 2024 06:06
@hzxa21
hzxa21 force-pushed the patrick/sink-rate-limit.pr branch 2 times, most recently from e656194 to e7217ba Compare December 19, 2024 08:44

@MrCroxx MrCroxx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@hzxa21
hzxa21 force-pushed the patrick/sink-rate-limit.pr branch from 45e0b1e to a9240fb Compare December 20, 2024 08:07
@MrCroxx

MrCroxx commented Dec 23, 2024

Copy link
Copy Markdown
Contributor

These logs are keep repeating. Let me try to fix.

2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed
2022-10-11T06:02:13.840593Z  WARN actor{otel.name="Actor 231" actor_id=231 prev_epoch=3160993419558912 curr_epoch=3160993541062656}:executor{otel.name="Sink E700000001"}: risingwave_connector::sink::log_store: rate limit control channel closed

Comment thread src/connector/src/sink/log_store.rs Outdated
paused = self.rate_limit == Some(0);
tracing::info!("rate limit changed from {:?} to {:?}, paused = {paused}", prev, self.rate_limit);
} else {
tracing::warn!("rate limit control channel closed");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the log reader exit here? Seems there is no way to restore the rate limit control channel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simulation recovery test passed with bail! here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it okay that I commit the fix directly in this PR?

@MrCroxx
MrCroxx added this pull request to the merge queue Dec 23, 2024
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Dec 23, 2024
@MrCroxx
MrCroxx added this pull request to the merge queue Dec 23, 2024
Merged via the queue into main with commit 4aab017 Dec 23, 2024
@MrCroxx
MrCroxx deleted the patrick/sink-rate-limit.pr branch December 23, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support rate limit of sink

4 participants