Tag TSM stats with database and retention policy #5844
Conversation
| @@ -112,11 +113,15 @@ type Cache struct { | |||
|
|
|||
| // NewCache returns an instance of a cache which will use a maximum of maxSize bytes of memory. | |||
| // Only used for engine caches, never for snapshots | |||
| func NewCache(maxSize uint64, path string) *Cache { | |||
| func NewCache(maxSize uint64, config tsdb.ShardConfig) *Cache { | |||
There was a problem hiding this comment.
This might be confusing to pass in the shard config to this function since only one of the two attributes included in the shard config are actually used. When using this function, you may wonder "why does it only care about one part of the config?"
I think it might be better to continue passing the path as a string directly here and to modify the call site in NewEngine to pass in the relevant string from the shard config. It will also make the diff a lot smaller and won't change the public interface.
There was a problem hiding this comment.
Agree w/ @jsternberg. I prefer that path args.
There was a problem hiding this comment.
The shard config contains 4 fields:
Path, which was previously used in NewCacheWALPath, which is ignored in NewCacheDatabaseandRetentionPolicy, which are now used as tags on the Cache (and Filestore and WAL) so that it's trivial to match up which Shards and which parts of the engine stats line up together
|
I missed the part where the database and retention policy from the shard config were being used since I only saw how the tests were modified. I retract my previous comments. 👍 |
|
After some out-of-band discussion, I'm going to amend the commit to continue passing |
mark-rushakoff
force-pushed
the
mr-tsm-stats-tag-db-rp
branch
from
1732014
to
ffa04f0
Compare
|
Rebased to address comments and now ready for re-review @jsternberg @jwilder. |
| path, _ := filepath.Split(filepath.Clean(shardOrWALPath)) | ||
|
|
||
| // Extract the database and retention policy. | ||
| path, rp := filepath.Split(filepath.Clean(path)) |
There was a problem hiding this comment.
It's probably not necessary to call clean multiple times. Should you just use filepath.SplitList?
There was a problem hiding this comment.
It is necessary. See golang/go#9928 for a slightly more detailed discussion.
filepath.SplitList is meant to parse out colon separated list of paths like you would see in your $PATH environment variable.
|
👍 |
|
Needs a changelog entry, but 👍 after that. |
| statMap: statMap, | ||
| LogOutput: os.Stderr, | ||
| } | ||
| } | ||
|
|
||
| // Path returns the path set on the shard when it was created. | ||
| func (s *Shard) Path() string { return s.config.Path } | ||
| func (s *Shard) Path() string { return s.path } |
There was a problem hiding this comment.
While this doesn't need a read lock on s.mu, I think it should either have one, or the one in DiskSize should be removed. There are no concurrent writes to s.path, so technically no lock is currently needed, but it would be better to be consistent either way..
If it's decided to add a read lock to Path() then you should also move path under mu in the Shard type definition.
|
I'll remove the read lock from |
mark-rushakoff
force-pushed
the
mr-tsm-stats-tag-db-rp
branch
from
ffa04f0
to
2e8de1c
Compare
... by extracting the db/rp from the given path. Now that the code has "standardized" on extracting db/rp this way, the ShardLocation struct is no longer necessary and thus has been removed. We're back on the previous style of passing the path and walPath to NewShard.
mark-rushakoff
force-pushed
the
mr-tsm-stats-tag-db-rp
branch
from
2e8de1c
to
cdcb079
Compare
Tag TSM stats with database and retention policy
(This probably should have been part of #5810. Hindsight is 20/20.)
The primary intent of this changeset is to tag stats for TSM cache, WAL, and filestore with the owner database and retention policy. The database and retention policy are inferred from the path supplied to the engine component.