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(query): Avoid race in pipeline delegator #2608

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Conversation

nikhars
Copy link
Member

@nikhars nikhars commented Apr 13, 2022

This fixes the race in pipeline delegator by making a copy of the query.
But there is an override mechanism via options wherein we can override
copying query for certain referrers. These would be the
subscription and subscriptions_executor referrers since they are
too sensitive to copying and they would be disallowed from running
parallel queries.

This fixes the race in pipeline delegator by making a copy of the query.
But there is an override mechanism via options wherein we can override
copying query for certain referrers. These would be the
subscription and subscriptions_executor referrers since they are
too sensitive to copying and they would be disallowed from running
parallel queries.
@nikhars nikhars requested a review from a team as a code owner April 13, 2022 17:22
@@ -26,6 +29,20 @@
]


def _is_query_copying_disallowed(current_referrer: str) -> bool:
"""
Check if the given referrer is disallowed from making a copy of the query.
Copy link
Member

Choose a reason for hiding this comment

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

maybe you could add why a given referrer would be disallowed from making a copy of the query? I'm not sure it's fully obvious to me why subscriptions and subscriptions executor are too sensitive for using deepcopy

@onewland
Copy link
Contributor

Too sensitive is referring to the increased query latency we saw on the original deploy of that change, right?

Copy link
Member

@evanh evanh left a comment

Choose a reason for hiding this comment

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

We need a better way of duplicating queries. That's a bigger problem though.

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #2608 (4081c2e) into master (359f11e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4081c2e differs from pull request most recent head f69ecf5. Consider uploading reports for the commit f69ecf5 to get more accurate results

@@           Coverage Diff           @@
##           master    #2608   +/-   ##
=======================================
  Coverage   92.75%   92.75%           
=======================================
  Files         606      606           
  Lines       28195    28213   +18     
=======================================
+ Hits        26152    26170   +18     
  Misses       2043     2043           
Impacted Files Coverage Δ
snuba/pipeline/pipeline_delegator.py 100.00% <100.00%> (ø)
tests/pipeline/test_pipeline_delegator.py 96.36% <100.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 359f11e...f69ecf5. Read the comment docs.

@nikhars nikhars merged commit 010c5dc into master Apr 14, 2022
@nikhars nikhars deleted the fix/create-query-copy branch April 14, 2022 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants