ref(subscriptions): Executor created within strategy

[MeredithAnya]

context
This is kind of a reverse and then some of: #2364 but for different reasons. Some background on this PR:

The task I'm currently working on is auto scaling our snuba consumers, starting with the executors. We have auto scaling on some of snuba services, such as the api¹, but not everything. Additionally, certain consumers, like the executors² have args passed in that depend on the no. of replicas (which will no longer will be constant if we have auto scaling implemented).

why replica dependent args not great?
When an autoscaler kicks in, it adds new pods. The new pods will be aware of new number of replicas, whereas the currently running "old" pods will not. This would lead to different parameters for different pods, and those parameters might not be appropriate any longer (which might also be the case if we scaled down)

Executors
The executors depend on max-concurrent-queries being passed in which is calculated from the replicas:

- --max-concurrent-queries
- "{{ (total_concurrent_queries / metrics_subscription_executor_replicas)|int }}"

We don't really want to restart every single pod each time the auto scaler kicks in just to update the max_concurrent_queries so we'd like max_concurrent_queries to be calculated within the executor itself. I have two options for how to do that, one with changes solely in snuba³ and one that would require arroyo changes⁴.

Both options however, would require that the executor be created within the ExecuteQuery strategy.

Footnotes

1. auto scaling example snuba api https://github.com/getsentry/ops/blob/5b33fe67830a8d9e36ea0a0b49a2371d4f9f93bd/k8s/services/snuba/_deployment.api.yaml#L17-L29 ↩

2. metrics executor config https://github.com/getsentry/ops/blob/5b33fe67830a8d9e36ea0a0b49a2371d4f9f93bd/k8s/services/snuba/_deployment.metrics-subscriptions-executor.yaml#L59-L60 ↩

3. https://github.com/getsentry/snuba/pull/2745 ↩

4. https://github.com/getsentry/snuba/pull/2761 ↩

[codecov-commenter]

Codecov Report

Merging #2762 (39f15c1) into master (cb96121) will increase coverage by 0.00%.
The diff coverage is 66.66%.

❗ Current head 39f15c1 differs from pull request most recent head 0541473. Consider uploading reports for the commit 0541473 to get more accurate results

@@           Coverage Diff           @@
##           master    #2762   +/-   ##
=======================================
  Coverage   92.82%   92.82%           
=======================================
  Files         609      609           
  Lines       28624    28611   -13     
=======================================
- Hits        26569    26558   -11     
+ Misses       2055     2053    -2     

Impacted Files	Coverage Δ
snuba/cli/subscriptions_executor.py	52.63% <0.00%> (+0.08%)	⬆️
snuba/cli/subscriptions_scheduler_executor.py	56.36% <0.00%> (+0.22%)	⬆️
snuba/subscriptions/combined_scheduler_executor.py	79.00% <ø> (-0.21%)	⬇️
.../subscriptions/test_combined_scheduler_executor.py	100.00% <ø> (ø)
snuba/subscriptions/executor_consumer.py	92.70% <100.00%> (ø)
tests/subscriptions/test_executor_consumer.py	100.00% <100.00%> (ø)

---

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 cb96121...0541473. Read the comment docs.