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

feat(subscriptions): Executor uses the ExecuteQuery strategy #2280

Merged
merged 4 commits into from
Dec 16, 2021

Conversation

lynnagara
Copy link
Member

Integrates the ExecuteQuery strategy in the subscriptions executor.
Note that currently the executor_sample_rate option is not set
so all scheduled subscriptions will be filtered at the start of the
pipeline, and even their offsets will not be committed.

This is to avoid running duplicate queries on ClickHouse until we
are ready to begin testing.

Integrates the ExecuteQuery strategy in the subscriptions executor.
Note that currently the `executor_sample_rate` option is not set
so all scheduled subscriptions will be filtered at the start of the
pipeline, and even their offsets will not be committed.

This is to avoid running duplicate queries on ClickHouse until we
are ready to begin testing.
@lynnagara lynnagara requested a review from a team as a code owner December 14, 2021 20:57
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2021

Codecov Report

Merging #2280 (acff423) into master (c614d1a) will decrease coverage by 0.13%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2280      +/-   ##
==========================================
- Coverage   93.20%   93.06%   -0.14%     
==========================================
  Files         551      553       +2     
  Lines       24989    25245     +256     
==========================================
+ Hits        23290    23494     +204     
- Misses       1699     1751      +52     
Impacted Files Coverage Δ
snuba/subscriptions/executor_consumer.py 89.51% <75.00%> (+0.53%) ⬆️
snuba/admin/views.py 63.49% <0.00%> (-16.51%) ⬇️
snuba/datasets/factory.py 85.36% <0.00%> (-1.60%) ⬇️
snuba/cli/subscriptions.py 62.96% <0.00%> (-1.52%) ⬇️
snuba/query/validation/validators.py 95.19% <0.00%> (-0.05%) ⬇️
tests/test_writer.py 100.00% <0.00%> (ø)
snuba/query/snql/parser.py 96.12% <0.00%> (ø)
tests/test_sessions_api.py 100.00% <0.00%> (ø)
tests/query/test_matcher.py 100.00% <0.00%> (ø)
tests/subscriptions/test_codecs.py 100.00% <0.00%> (ø)
... and 18 more

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 c614d1a...acff423. Read the comment docs.

def create(
self, commit: Callable[[Mapping[Partition, Position]], None]
) -> ProcessingStrategy[KafkaPayload]:
return Noop(commit)
executor = ThreadPoolExecutor(self.__max_concurrent_queries)
Copy link
Contributor

Choose a reason for hiding this comment

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

You do not need to create a threadpool each time you create the step, which can happen often. Please keep it static.

Copy link
Member Author

@lynnagara lynnagara Dec 15, 2021

Choose a reason for hiding this comment

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

Moved this line out of the processing strategy factory and passing it in now so it doesn't get recreated on partition assignment, etc

@lynnagara lynnagara merged commit a9c82ff into master Dec 16, 2021
@lynnagara lynnagara deleted the integrate-execute-query-strategy branch December 16, 2021 18:50
lynnagara added a commit that referenced this pull request Dec 16, 2021
Integrates the ExecuteQuery strategy in the subscriptions executor.
Note that currently the `executor_sample_rate` option is not set
so all scheduled subscriptions will be filtered at the start of the
pipeline, and even their offsets will not be committed.

This is to avoid running duplicate queries on ClickHouse until we
are ready to begin testing.
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.

3 participants