-
Notifications
You must be signed in to change notification settings - Fork 702
dq/runtime/output_channel: relax requirement on double watermark/checkpoint #21459
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
base: main
Are you sure you want to change the base?
dq/runtime/output_channel: relax requirement on double watermark/checkpoint #21459
Conversation
assumption was incorrect; if channel is not ready double watermark can be pushed into
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
🟢 |
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
@@ -478,7 +482,7 @@ class TDqOutputChannel : public IDqOutputChannel { | |||
bool Finished = false; | |||
|
|||
TMaybe<NDqProto::TWatermark> Watermark; | |||
TMaybe<NDqProto::TCheckpoint> Checkpoint; | |||
TDeque<NDqProto::TCheckpoint> Checkpoints; |
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.
2 чекпойнта возможно только если inflight в конфиге больше 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.
Есть edge case: в кверях с записью не в топик, результат записывается в канал.
А новый checkpoint запускается после того, как checkpoint прошёл по всем таскам и все sink отчитались от записи.
В данном случае синков нет, последняя таска записала в канал, если канал при этом почему-то застрял -- в него может прилететь второй checkpoint.
Городить ради этого TDeque
немножко жалко, но[...]
Changelog entry
...
Changelog category
Description for reviewers
Fixes YQ-4351. While two cases are similar, they are handled a bit differently.
In case of checkpoint, we cannot lose checkpoints, and we cannot reorder checkpoints and data. So, if channel contains any untransferred checkpoints, allow adding more checkpoints, but with no data between.
In case of watermarks, we can move watermark behind new data and merge several watermarks.