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

Fix race conditions in updating RemoteTaskStats #11044

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

raunaqmorarka
Copy link
Member

Description

In the existing implementation, the shared currentRequestStartNanos object
may be updated by the next request after the current request future is done
but before the callback handler has read the value of currentRequestStartNanos
set before the current request was started.

General information

Is this change a fix, improvement, new feature, refactoring, or other?

Fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Core query engine

How would you describe this change to a non-technical end user or system administrator?

Fixes potential inaccuracies in recording of RemoteTaskStats

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@findepi
Copy link
Member

findepi commented Feb 15, 2022

@raunaqmorarka is this related to #11039 ?

@raunaqmorarka
Copy link
Member Author

@findepi I don't think this will resolve the value is negative failures. It's just going to avoid inaccuracies in stats due to a possible race condition.

In the existing implementation, the shared currentRequestStartNanos object
may be updated by the next request after the current request future is done
but before the callback handler has read the value of currentRequestStartNanos
set before the current request was started.
Copy link
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

From what I understand ContinuousTaskStatusFetcher wasn't susceptible to this issue as a new request is always scheduled from a callback, but DynamicFiltersFetcher and TaskInfoFetcher were

@losipiuk losipiuk merged commit fe3d23a into trinodb:master Feb 17, 2022
@losipiuk
Copy link
Member

Merged, thanks!

@github-actions github-actions bot added this to the 372 milestone Feb 17, 2022
@raunaqmorarka raunaqmorarka deleted the nano-race branch February 21, 2022 13:12
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