-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[flink] Bypass operator should never block checkpoint #4039
[flink] Bypass operator should never block checkpoint #4039
Conversation
output.collect(new StreamRecord<>(Either.Right(task))); | ||
} | ||
|
||
compactTasks.addAll(tasks); |
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.
Should we first check if tasks.isEmpty()? If it's not empty, then compactTasks.addAll(tasks);
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.
Why?
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 if tasks.isEmpty(), then there is no need to execute compactTasks.addAll (tasks);
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.
So what is the benefit?
@Override | ||
public void onProcessingTime(long time) { | ||
while (true) { | ||
public void asyncPlan(UnawareAppendTableCompactionCoordinator coordinator) { |
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.
private
Hi, thx for this @JingsongLi I have a doubt that you say generating compact tasks blocking normal threads and even resulting in checkpoint delays. So, should we support both this new topology and old union topology simultaneously? |
What is your point? You mean you use new code and the job still back pressure? |
ok,after merge,I will test soon. |
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.
+1
Purpose
The previous
AppendBypassCoordinateOperator
spent a lot of time generating compact tasks when there were many files, leading to significant blocking of normal threads and even resulting in checkpoint delays.Modify it:
Tests
API and Format
Documentation