Standardize metrics for task isolation leaking and include cause #6562
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
natemort
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Shaddoll
reviewed
Dec 14, 2024
| @@ -74,14 +74,12 @@ func NewDefaultIsolationGroupStateWatcherWithConfigStoreClient( | |||
| }, nil | |||
| } | |||
|
|
|||
| func (z *defaultIsolationGroupStateHandler) AvailableIsolationGroupsByDomainID(ctx context.Context, domainID string, tasklistName string, availablePollerIsolationGroups []string) (types.IsolationGroupConfiguration, error) { | |||
| func (z *defaultIsolationGroupStateHandler) AvailableIsolationGroupsByDomainID(ctx context.Context, domainID string, _ string) (types.IsolationGroupConfiguration, error) { | |||
There was a problem hiding this comment.
the behavior of this method changed
previously, it only returned undrained isolation groups but now it also returns drained groups
consider renaming it or doing it in a different way if you want to get drained groups
taylanisikdemir
approved these changes
Dec 17, 2024
timl3136
approved these changes
Dec 17, 2024
Shaddoll
approved these changes
Dec 17, 2024
| @@ -43,5 +43,5 @@ type State interface { | |||
|
|
|||
| // AvailableIsolationGroupsByDomainID returns the available isolation zones for a domain. | |||
| // Takes into account global and domain zones | |||
| AvailableIsolationGroupsByDomainID(ctx context.Context, domainID string, tasklistName string, availableIsolationGroups []string) (types.IsolationGroupConfiguration, error) | |||
| IsolationGroupsByDomainID(ctx context.Context, domainID string) (types.IsolationGroupConfiguration, error) | |||
There was a problem hiding this comment.
maybe mind adding an updated comment about what the purpose of the interface is? eg "it's responsibility as a component is to return the isolation group for a domain"?
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.
What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes