Skip to content

Rules: Refactor concurrency controller interface#14491

Merged
gotjosh merged 6 commits intomainfrom
modify-interface-for-concurrency-controller
Jul 22, 2024
Merged

Rules: Refactor concurrency controller interface#14491
gotjosh merged 6 commits intomainfrom
modify-interface-for-concurrency-controller

Conversation

@gotjosh
Copy link
Copy Markdown
Member

@gotjosh gotjosh commented Jul 19, 2024

Even though the main purpose of this refactor is to modify the interface of the concurrency controller to accept a Context. I did two drive-by modifications that I think are sensible:

  1. I have moved the check for dependencies on rules to the controller itself. This aligns with how the controller should behave, as it is a deciding factor in whether we should run concurrently or not.
  2. I cleaned up some unused methods from the days of the old interface before Decouple ruler dependency controller from concurrency controller #13527 changed it.

gotjosh added 2 commits July 19, 2024 15:12
Even though the main purpose of this refactor is to modify the interface of the concurrency controller to accept a Context. I did two drive-by modifications that I think are sensible:

1. I have moved the check for dependencies on rules to the controller itself - this aligns with how the controller should behave as it is a deciding factor on wether we should run concurrently or not.
2. I cleaned up some unused methods from the days of the old interface before #13527 changed it.

Signed-off-by: gotjosh <[email protected]>
@gotjosh gotjosh marked this pull request as draft July 19, 2024 16:13
Copy link
Copy Markdown
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM

@gotjosh gotjosh marked this pull request as ready for review July 22, 2024 09:57
@gotjosh
Copy link
Copy Markdown
Member Author

gotjosh commented Jul 22, 2024

This doesn't change any functionality and is just a simple refactor - I'm going to merge for now but please do let me know if this there's something that I missed.

@gotjosh gotjosh enabled auto-merge (squash) July 22, 2024 12:58
@gotjosh gotjosh disabled auto-merge July 22, 2024 12:58
Copy link
Copy Markdown
Member

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

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

LGTM!!

Not sure how widespread RuleConcurrencyController interface is but this change is modifying it. The contributor that makes the next release should probably add it to the changelog.

@gotjosh
Copy link
Copy Markdown
Member Author

gotjosh commented Jul 22, 2024

Not sure how widespread RuleConcurrencyController interface

It's only used on an experimental feature and as far as I know not used yet by any downstream projects as it is relatively new.

@gotjosh gotjosh merged commit 465891c into main Jul 22, 2024
@gotjosh gotjosh deleted the modify-interface-for-concurrency-controller branch July 22, 2024 13:11
kushalShukla-web pushed a commit to kushalShukla-web/prometheus that referenced this pull request Jul 23, 2024
* Rules: Refactor concurrency controller interface

Even though the main purpose of this refactor is to modify the interface of the concurrency controller to accept a Context. I did two drive-by modifications that I think are sensible:

1. I have moved the check for dependencies on rules to the controller itself - this aligns with how the controller should behave as it is a deciding factor on wether we should run concurrently or not.
2. I cleaned up some unused methods from the days of the old interface before prometheus#13527 changed it.

Signed-off-by: gotjosh <[email protected]>
---------

Signed-off-by: gotjosh <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
kushalShukla-web pushed a commit to kushalShukla-web/prometheus that referenced this pull request Jul 25, 2024
* Rules: Refactor concurrency controller interface

Even though the main purpose of this refactor is to modify the interface of the concurrency controller to accept a Context. I did two drive-by modifications that I think are sensible:

1. I have moved the check for dependencies on rules to the controller itself - this aligns with how the controller should behave as it is a deciding factor on wether we should run concurrently or not.
2. I cleaned up some unused methods from the days of the old interface before prometheus#13527 changed it.

Signed-off-by: gotjosh <[email protected]>
---------

Signed-off-by: gotjosh <[email protected]>
kushalShukla-web pushed a commit to kushalShukla-web/prometheus that referenced this pull request Jul 25, 2024
* Rules: Refactor concurrency controller interface

Even though the main purpose of this refactor is to modify the interface of the concurrency controller to accept a Context. I did two drive-by modifications that I think are sensible:

1. I have moved the check for dependencies on rules to the controller itself - this aligns with how the controller should behave as it is a deciding factor on wether we should run concurrently or not.
2. I cleaned up some unused methods from the days of the old interface before prometheus#13527 changed it.

Signed-off-by: gotjosh <[email protected]>
---------

Signed-off-by: gotjosh <[email protected]>
Signed-off-by: Kushal Shukla <[email protected]>
bboreham pushed a commit to bboreham/prometheus that referenced this pull request Oct 22, 2024
…r-prometheus-b03b895

* upstream/main:
  [DOCS] Querying basics: explain range and instant queries
  Rules: Refactor concurrency controller interface (prometheus#14491)
  [PRW 2.0] Added Sender and RW Handler support for Response Stats. (prometheus#14444)
  docs: Correct and clarify histogram bucket and resolution limits
  OTLPConfig.UnmarshalYAML: Return error on invalid input
  Update for Docker deprecation
  Bump Otel semconv version to 1.26.0
  Update Go dependencies
  Add otlptranslator tests
  Add to changelog
  Add config tests
  Sanitize configured OTel resource attributes
  promql: Add NHCB tests
  prometheusremotewrite: Support resource attribute promotion
  Update storage.md to provide right-sizing advice on retention
  TSDB: shrink memSeries by moving bools together
  ci: Add job to report build_all status
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.

4 participants