Clean up shard data when dropping retention policies #5691
Conversation
e-dard
force-pushed
the
er-retention-policies
branch
from
ada132a to
f57e2a0
Compare
|
Looks like this will fix #5394 as well. |
| @@ -252,6 +261,7 @@ func (s *Store) CreateShard(database, retentionPolicy string, shardID uint64) er | |||
| } | |||
|
|
|||
| s.shards[shardID] = shard | |||
| s.shardLocations[shardID] = &shardLocation{Database: database, RetentionPolicy: retentionPolicy, Shard: shard} | |||
There was a problem hiding this comment.
Any reason not to just combine shards and shardLocations maps? shardLocation has a pointer to the Shard which is all that shards has.
There was a problem hiding this comment.
Yeah that was my thinking, but I wanted to get feedback on whether it's OK to do away with shards first. Seems I forgot to even mention that in the PR description 😄, so thanks for bring it up!
I'll refactor this morning.
e-dard
force-pushed
the
er-retention-policies
branch
2 times, most recently
from
9e73c57 to
be78c8c
Compare
e-dard
changed the title
e-dard
force-pushed
the
er-retention-policies
branch
from
be78c8c to
241a92b
Compare
| for _, sh := range s.shards { | ||
| s.performMaintenanceOnShard(sh) | ||
| for _, sl := range s.shardLocations { | ||
| s.performMaintenanceOnShard(sl.Shard) |
There was a problem hiding this comment.
Does this need to check that sl.Shard is not nil as well? It's done in other places.
There was a problem hiding this comment.
I guess, yeah. To be honest a shardLocation shouldn't have a nil Shard, but better to be defensive 👍
|
One last item. But 👍 after that. |
e-dard
force-pushed
the
er-retention-policies
branch
from
241a92b to
012b89c
Compare
| for _, sh := range s.shards { | ||
| if err := sh.Close(); err != nil { | ||
| for _, sl := range s.shardLocations { | ||
| if err := sl.Shard.Close(); err != nil { |
There was a problem hiding this comment.
Possibly check for nil? Although NewShard() currently never returns nil, and that appears to be the only value that gets stored in sl.Shard, I see that you've checked for nil in other places for this field.
There was a problem hiding this comment.
Or, (*Shard).Close() can be updated to handle a nil receiver.
Fixes #5653 and #5394. Previously dropping retention policies did not propogate to local TSDB shards. Instead, the retention policiess would just be removed from the Meta Store. This PR adds ensures that data associated with retention policies is removed, when the retention policy is dropped. Also, it cleans up a couple of other methods in `tsdb`, including the requirement to provide (redundant) shardIDs when deleting databases.
e-dard
force-pushed
the
er-retention-policies
branch
from
012b89c to
99a7341
Compare
|
Discussed defensive |
Clean up shard data when dropping retention policies
Fixes #5653.
tsdb.Storefor deleting shards in a retention policy, and cleaning up said retention policy.tsdb.Store.DeleteDatabaseso that it does not require the ids of the shards within the database.