Add sharding strategy support #20
Conversation
QuentinBisson
force-pushed
the
add-sharding-strategy-support
branch
2 times, most recently
from
659b618 to
a9ada66
Compare
QuentinBisson
force-pushed
the
add-sharding-strategy-support
branch
2 times, most recently
from
0d9c03a to
26e1691
Compare
|
@giantswarm/team-atlas I'd love some eyes ;) |
| ScaleDownPercentage float64 | ||
| } | ||
|
|
||
| func (pass1 ShardingStrategy) Merge(pass2 *ShardingStrategy) ShardingStrategy { |
There was a problem hiding this comment.
We can use a pointer here and just modify the current object instead of returning a new one.
| func (pass1 ShardingStrategy) Merge(pass2 *ShardingStrategy) ShardingStrategy { | |
| func (s *ShardingStrategy) Merge(newShardingStrategy *ShardingStrategy) { |
There was a problem hiding this comment.
I prefer immutability because I do not want to change the origin object. I suppose it would be fine as I construct a new strategy from the monitoring config but if we create the sharding strategy as a member of the config then we should not use a pointer.
Also, I'm not sure creating a few objects once every 12 h is really a concern :D
There was a problem hiding this comment.
It's not a concerns in terms of performance, just trying to simplify the code for the readers.
| strategy := ShardingStrategy{ | ||
| pass1.ScaleUpSeriesCount, | ||
| pass1.ScaleDownPercentage, | ||
| } |
There was a problem hiding this comment.
| strategy := ShardingStrategy{ | |
| pass1.ScaleUpSeriesCount, | |
| pass1.ScaleDownPercentage, | |
| } |
| if pass2 != nil { | ||
| if pass2.ScaleUpSeriesCount > 0 { | ||
| strategy.ScaleUpSeriesCount = pass2.ScaleUpSeriesCount | ||
| } | ||
| if pass2.ScaleDownPercentage > 0 { | ||
| strategy.ScaleDownPercentage = pass2.ScaleDownPercentage | ||
| } | ||
| } | ||
| return strategy |
There was a problem hiding this comment.
| if pass2 != nil { | |
| if pass2.ScaleUpSeriesCount > 0 { | |
| strategy.ScaleUpSeriesCount = pass2.ScaleUpSeriesCount | |
| } | |
| if pass2.ScaleDownPercentage > 0 { | |
| strategy.ScaleDownPercentage = pass2.ScaleDownPercentage | |
| } | |
| } | |
| return strategy | |
| if newShardingStrategy != nil { | |
| if newShardingStrategy.ScaleUpSeriesCount > 0 { | |
| s.ScaleUpSeriesCount = newShardingStrategy.ScaleUpSeriesCount | |
| } | |
| if newShardingStrategy.ScaleDownPercentage > 0 { | |
| s.ScaleDownPercentage = newShardingStrategy.ScaleDownPercentage | |
| } | |
| } |
| shardingStrategy := sharding.ShardingStrategy{ | ||
| ScaleUpSeriesCount: pas.MonitoringConfig.ShardingScaleUpSeriesCount, | ||
| ScaleDownPercentage: pas.MonitoringConfig.ShardingScaleDownPercentage, | ||
| }.Merge(clusterShardingStrategy) |
There was a problem hiding this comment.
| shardingStrategy := sharding.ShardingStrategy{ | |
| ScaleUpSeriesCount: pas.MonitoringConfig.ShardingScaleUpSeriesCount, | |
| ScaleDownPercentage: pas.MonitoringConfig.ShardingScaleDownPercentage, | |
| }.Merge(clusterShardingStrategy) | |
| shardingStrategy := pas.MonitoringConfig | |
| shardingStrategy.Merge(clusterShardingStrategy) |
There was a problem hiding this comment.
MonitoringConfig is a struct. Merge will not work
There was a problem hiding this comment.
We could however create a shardingstrategy in the config instead
QuentinBisson
force-pushed
the
add-sharding-strategy-support
branch
from
efa2965 to
6ac51b6
Compare
|
@TheoBrigitte could you take another look? I think I addressed all your concerns :) |
QuentinBisson
force-pushed
the
add-sharding-strategy-support
branch
2 times, most recently
from
959f2e0 to
6d92c26
Compare
| ) | ||
|
|
||
| func TestShardComputationScaleUp(t *testing.T) { | ||
| pass := Strategy{ScaleUpSeriesCount: float64(1_000_000), ScaleDownPercentage: float64(0.20)} |
There was a problem hiding this comment.
Nice test coverage, but for multiple test cases we usually wrap test cases into a slice and iterate over via t.Run
There was a problem hiding this comment.
Can we pair on this on thursday ? I quite like having functions as the name make for a nice description but i'm eager to learn how to bé more idiomatic :)
Signed-off-by: QuentinBisson <[email protected]>
Co-authored-by: Théo Brigitte <[email protected]>
Co-authored-by: Théo Brigitte <[email protected]>
Co-authored-by: Théo Brigitte <[email protected]>
Co-authored-by: Théo Brigitte <[email protected]>
Signed-off-by: QuentinBisson <[email protected]>
QuentinBisson
force-pushed
the
add-sharding-strategy-support
branch
from
6d92c26 to
eeea937
Compare
| }, | ||
| } | ||
|
|
||
| func TestShardComputationLogic(t *testing.T) { |
There was a problem hiding this comment.
@TheoBrigitte let me know what you think now
Signed-off-by: QuentinBisson <[email protected]>
QuentinBisson
force-pushed
the
add-sharding-strategy-support
branch
from
051a347 to
f61bb41
Compare
What this PR does / why we need it
This PR adds the code that was added to PMO towards this issue https://github.com/giantswarm/giantswarm/issues/30673 to be able to configure the sharding strategy for some installations or some clusters only
Checklist