-
Notifications
You must be signed in to change notification settings - Fork 787
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
Adds partitioning library #5169
Adds partitioning library #5169
Conversation
davidporter-id-au
commented
Mar 20, 2023
- Adds an interface for handling workflow isolation group redirection and failover
- Adds an interface for handling drain states for both global and domain-based drains
- Adds relevant internal types
- Adds a simple interface for handling global drains as stored in database-based dynamic config
- Adds a default implementation for the open-source implementation and implementation example
- Adds an interface for handling workflow isolation group redirection and failover - Adds an interface for handling drain states for both global and domain-based drains - Adds relevant internal types - Adds a simple interface for handling global drains as stored in database-based dynamic config - Adds a default implementation for the open-source implementation and implementation example
|
||
// it's drained, fall back to picking a deterministic but random group | ||
availableList := []string{} | ||
for k, v := range available { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
available
seems to only contain healthy isolation groups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the intention, yes? I'm not quite sure I understand your comment.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it only contains healthy groups, we don't need the if condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, sorry, missed that, yep
// it's drained, fall back to picking a deterministic but random group | ||
availableList := []string{} | ||
for k, v := range available { | ||
if v.State == types.IsolationGroupStateDrained { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest changing the condition to be v.State != types.IsolationGroupStateHealthy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a deny-based implementation. we're only skipping zones which have been drained. Let me sync to make sure we're on the same page here.
// drain. | ||
func pickIsolationGroup(wfPartition defaultWorkflowPartitionConfig, available types.IsolationGroupConfiguration) string { | ||
wfIG, isAvailable := available[wfPartition.WorkflowStartIsolationGroup] | ||
if isAvailable && wfIG.State != types.IsolationGroupStateDrained { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest changing the condition to be wfIG.State == types.IsolationGroupStateHealthy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a deny-based implementation. Isolation groups are explicitly assumed to be healthy unless specified as drained, so I can't rely on their being configured here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the state is not a boolean, we might introduce other state that is not healthy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's possible, but I'm maybe not understanding your point. Actually explicitly healthy groups will be not included in this list at all under this implementation, so picking healthy explicitly is not an option.
return nil, fmt.Errorf("could not resolve domain in isolationGroup handler: %w", err) | ||
} | ||
domainState := domainData.GetInfo().IsolationGroupConfig | ||
globalCfg, err := z.globalIsolationGroupDrains.FetchConfig(ctx, persistence.GlobalIsolationGroupConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need some cache for configStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but if it's ok let me follow it up. I'd like to do that in a subsequent PR
common/partition/interface.go
Outdated
|
||
// Config is the base configuration for the partitioning library | ||
type Config struct { | ||
ZonalPartitioningEnabledGlobally dynamicconfig.BoolPropertyFnWithDomainIDFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need both ZonalPartitioningEnabledGlobally
and ZonalPartitioningEnabledForDomain
?
And we might need a better name, like EnableTasklistIsolation
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that name is out of date, let me fix, sorry. I would normally, for pure flipr, just use the one switch, but we probably can't push arbitrary values with overrides there without a bigger refactor
common/isolationgroup/types.go
Outdated
// Config is the base configuration | ||
type Config struct { | ||
ZonalPartitioningEnabledGlobally dynamicconfig.BoolPropertyFnWithDomainIDFilter | ||
ZonalPartitioningEnabledForDomain dynamicconfig.BoolPropertyFnWithDomainFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need both dynamic config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would we represent rollout on both a global and domain level? I'm totally open to the idea that maybe we don't care about this level of granularity, but I thought for rollout it's sufficiently high-risk that enabling it on a per-domain-basis might make sense.
common/partition/interface.go
Outdated
|
||
// Config is the base configuration for the partitioning library | ||
type Config struct { | ||
IsolationGroupEnabledGlobally dynamicconfig.BoolPropertyFnWithDomainIDFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not removed