Introduce ability to specify strategies for target allocation #1079
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Aneurysm9
reviewed
Sep 8, 2022
secustor
reviewed
Sep 8, 2022
| @@ -0,0 +1,91 @@ | |||
| package strategy | |||
There was a problem hiding this comment.
I think we should move the code in the strategy package back to allocation. CollectorJSON is only used in allocation/http.go and the other JSON structs are also around specifically for the http responses. It would also make naming make more sense (allocation.New() instead of strategy.New()). It feels like we're splitting up code that we don't really need to split up. I think that separating the code in this way makes it a little more difficult to navigate and figure out what's going on.
There was a problem hiding this comment.
I agree, i think a flat strategy here makes more sense for this codebase as it allows us to only export what's necessary
Aneurysm9
reviewed
Sep 9, 2022
Aneurysm9
approved these changes
Sep 9, 2022
kristinapathak
approved these changes
Sep 11, 2022
secustor
requested changes
Sep 12, 2022
…nto pluggable-strategies-take-two
pavolloffay
approved these changes
Sep 12, 2022
6 tasks
ItielOlenick
pushed a commit
to ItielOlenick/opentelemetry-operator
that referenced
this pull request
May 1, 2024
…elemetry#1079) * Cribbed from another branch * Refactor part 2 * Fix tests * Remove deadlocking problem * Update based on comments * Unexported some fields, added blocker and comments about invariants * Comments and imports fixed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following from #1068 ...
Goals:
Summary
This PR introduces the Allocator interface, implements it through a refactor of the current allocator, and changes the configuration of the TA to allow for a user to specify an option. The operator's CRDs are also updated to allow the strategy to be specified in the Collector CRD. New tests have been introduced to confirm the strategy works as expected. All previous tests should continue working.