Enable write autoscaling for active DynamoDB tables #507
Conversation
aaron7
changed the title
pkg/chunk/dynamodb_table_client.go
Outdated
| Help: "Time spent doing ApplicationAutoScaling requests.", | ||
|
|
||
| // ApplicationAutoScaling latency seems to range from a few ms to a few sec and is | ||
| // important. So use 8 buckets from 128us to 2s. |
There was a problem hiding this comment.
Actually true, or copy-pasted?
There was a problem hiding this comment.
This is copied and pasted from another AWS request metric. We will not know for sure what the latency will be until in dev/prod. I believe the requests are non-blocking as they are just setting configs which a watcher is then using later to scale the tables. I would expect the latency to be a standard AWS latency.
I will add a TODO here to check this later.
pkg/chunk/dynamodb_table_client.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure, but I think I'd write it like this:
var err error
if !current.WriteScaleEnabled {
if expected.WriteScaleEnabled {
err = d.enableAutoScaling(ctx, expected)
}
} else {
if !expected.WriteScaleEnabled {
err = d.disableAutoScaling(ctx, expected)
} else if !current.AutoScalingEquals(expected) {
err = d.enableAutoScaling(ctx, expected)
}
}
if err != nil {
return err
}But I can't articulate why, and am not sure it's better. WDYT?
There was a problem hiding this comment.
Agreed. I originally extracted out the expressions into variables to make it clear what each path was for, as it may take some time to get your head around why the combination of flags mean that we should either enable or disable. However, it's not that complex and it's clear which path is update and which one is disable, so I will update.
pkg/chunk/dynamodb_table_client.go
Outdated
| @@ -208,3 +304,95 @@ func (d dynamoTableClient) UpdateTable(ctx context.Context, current, expected Ta | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func (d dynamoTableClient) enableAutoScaling(ctx context.Context, desc TableDesc) error { | |||
| // Registers or updates a scallable target | |||
pkg/chunk/dynamodb_table_client.go
Outdated
| } | ||
|
|
||
| func (d dynamoTableClient) disableAutoScaling(ctx context.Context, desc TableDesc) error { | ||
| // Deregister scallable target |
pkg/chunk/table_client.go
Outdated
| WriteScaleMaxCapacity int64 | ||
| WriteScaleOutCooldown int64 | ||
| WriteScaleInCooldown int64 | ||
| WriteScaleTargetValue float64 |
There was a problem hiding this comment.
This looks the same as what's in schema_config.go. Why not split these fields into a separate struct type?
Then AutoScalingEquals can become Equals on that object (and maybe even the implementation could be replaced with a call to DeepEquals)
pkg/chunk/table_client.go
Outdated
| } | ||
| } | ||
| return true | ||
| } |
|
Thanks for the comments, ready for review again. |
pkg/chunk/schema_config.go
Outdated
| table.WriteScale.MaxCapacity = cfg.WriteScale.MaxCapacity | ||
| table.WriteScale.OutCooldown = cfg.WriteScale.OutCooldown | ||
| table.WriteScale.InCooldown = cfg.WriteScale.InCooldown | ||
| table.WriteScale.TargetValue = cfg.WriteScale.TargetValue |
There was a problem hiding this comment.
Why not just
table.WriteScale = cfg.WriteScale
pkg/chunk/table_manager.go
Outdated
| legacyTable.WriteScale.MaxCapacity = m.cfg.IndexTables.WriteScale.MaxCapacity | ||
| legacyTable.WriteScale.OutCooldown = m.cfg.IndexTables.WriteScale.OutCooldown | ||
| legacyTable.WriteScale.InCooldown = m.cfg.IndexTables.WriteScale.InCooldown | ||
| legacyTable.WriteScale.TargetValue = m.cfg.IndexTables.WriteScale.TargetValue |
Fixes #476. Original plan: #476 (comment)