Skip to content
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

Do not compare finished and all tasks sets eagerly #14635

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

sopel39
Copy link
Member

@sopel39 sopel39 commented Oct 14, 2022

No description provided.

@@ -395,7 +396,11 @@ private synchronized boolean isStageFlushing()

private synchronized boolean isStageFinished()
{
return finishedTasks.containsAll(allTasks);
boolean finished = finishedTasks.size() == allTasks.size();
Copy link
Member

Choose a reason for hiding this comment

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

If we are 100% sure that finishedTasks will not contain any additional tasks which are not included in allTasks this is ok, but I would think of >= comparison here.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are 100% sure that finishedTasks will not contain any additional tasks which are not included in allTasks

That would be a bug

@@ -395,7 +396,11 @@ private synchronized boolean isStageFlushing()

private synchronized boolean isStageFinished()
Copy link
Member

Choose a reason for hiding this comment

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

Can we do similar optimization in isStageFlushing ?

Copy link
Member

@Dith3r Dith3r Oct 17, 2022

Choose a reason for hiding this comment

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

isStageFlushing we check two different sets. There is a high chance that flushing size and finished size will be higher than allTask size (first we add to finished, and then we remove from flushing).

Copy link
Member

Choose a reason for hiding this comment

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

Right, if we can have a task id in both sets at some point in time, then we can't make similar optimization there.

@sopel39 sopel39 merged commit ca8e55b into trinodb:master Oct 17, 2022
@sopel39 sopel39 deleted the ks/do_not_compare branch October 17, 2022 13:11
@github-actions github-actions bot added this to the 401 milestone Oct 17, 2022
@colebow
Copy link
Member

colebow commented Oct 19, 2022

Does this need a release note?

@sopel39
Copy link
Member Author

sopel39 commented Oct 19, 2022

Does this need release notes?

This doesn't need a RN.

adding the no-release-notes label, or just writing "no release notes" in the PR description.

Will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants