storage: add replicated locks to MVCCStats #111293
Conversation
nvanbenschoten
requested review from
herkolategan,
renatolabs and
sumeerbhola
and removed request for
a team
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/replLocksMVCCStats
branch
from
4124b2d to
819c566
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 19 of 19 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @nvanbenschoten, @renatolabs, and @sumeerbhola)
pkg/storage/enginepb/mvcc.proto line 213 at r2 (raw file):
// table keyspace. Intent metadata keys are tracked under key_bytes // and their values are tracked under val_bytes. This is not to be // confused with the provisional versioned values protected by the
nit: s/protected/locked?
(in the commit message as well)
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 15 of 19 files at r2, all commit messages.
Reviewable status:
-- commits line 4 at r1:
Is LockAge used somewhere to try to do intent resolution for abandoned locks?
-- commits line 30 at r2:
MVCCStats is confusing. I think part of the problem is that the code comments in MVCCStats are written as if (a) the scheme is normal and obvious, (b) there is no example showing older versions, latest version, latest provisional value with an intent and clearly saying where the various bytes will be accounted.
It would be awesome if the comment deficiency could be fixed by this commit, given that it is adding LockBytes which also has to behave in a peculiar manner due to the legacy behavior.
pkg/storage/mvcc.go line 1486 at r2 (raw file):
func (b *putBuffer) lockTableKey( key roachpb.Key, str lock.Strength, txnID uuid.UUID, ) (EngineKey, int64) {
nit: can you name the return value since the meaning of an int64 is not obvious.
pkg/storage/enginepb/mvcc.go line 137 at r1 (raw file):
ms.GCBytesAge += ms.GCBytes() * diffSeconds ms.LockAge += ms.LockCount * diffSeconds
what will happen in a mixed version cluster where a newer version node will calculate differently. Is it harmless in that IntentCount==LockCount until the cluster version is updated?
This commit switches MVCCStats.AgeTo to derive the lock age from LockCount instead of IntentCount. Before replicated locks, LockCount and IntentCount were identical. With replicated locks, LockCount also includes shared and exclusive locks, so it's a better measure of the age of locks in a range. Release note: None
Fixes cockroachdb#109645. Informs cockroachdb#100193. This commit adds `MVCCStats` for replicated locks. To do so, it first adds a new field to the stats struct, `LockBytes`. `LockBytes` is the encoded size of replicated locks with shared or exclusive strengths, which are stored in the lock table keyspace. The field includes the size of the locks' keys and their values. For historical reasons, the field excludes the size of intent metadata key-values, even though they are also stored in the lock table keyspace. Intent metadata keys are tracked under KeyBytes and their values are tracked under ValBytes. This is not to be confused with the provisional versioned values protected by the intents, which are tracked by the IntentBytes field (and also by KeyBytes and ValBytes). Hence the vague "without their meta keys" comment above. The patch then begins accounting for the contributions of replicated locks to `LockBytes`, `LockCount`, and `LockAge`, of which the second two fields already exist. This accounting is straightforward. The less straightforward part of the patch is MVCC stats computation. Scanning the lock table requires the use of an EngineIterator. To this point, all stats computation has taken place on an MVCCIterator. The patch addresses this by directly scanning the lock table with an EngineIterator (wrapped in a LockTableIterator) during stats computation. Release note: None
This commit adds the storage metrics `lockbytes` and `lockcount` for replicated locks. They mirror `intentbytes` and `intentcount`, though their meaning is not directly analogous. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/replLocksMVCCStats
branch
from
819c566 to
cf11b77
Compare
There was a problem hiding this comment.
TFTRs!
Reviewable status:
Previously, sumeerbhola wrote…
Is LockAge used somewhere to try to do intent resolution for abandoned locks?
Yes, it is used by the GC queue to score ranges (see makeMVCCGCQueueScoreImpl), which determines whether they are queued for processing and, if so, whether they are prioritized. @arulajmani is in the process of hooking the GC queue up to actually resolve abandoned locks.
Previously, sumeerbhola wrote…
MVCCStats is confusing. I think part of the problem is that the code comments in MVCCStats are written as if (a) the scheme is normal and obvious, (b) there is no example showing older versions, latest version, latest provisional value with an intent and clearly saying where the various bytes will be accounted.
It would be awesome if the comment deficiency could be fixed by this commit, given that it is addingLockByteswhich also has to behave in a peculiar manner due to the legacy behavior.
I agree, but also want to avoid adding a sizable amount of work here to give this documentation effort the time that it deserves. I opened #111391 and will commit to spending some time on it once the release branch is cut and we're in stability.
pkg/storage/mvcc.go line 1486 at r2 (raw file):
Previously, sumeerbhola wrote…
nit: can you name the return value since the meaning of an int64 is not obvious.
Done.
pkg/storage/enginepb/mvcc.go line 137 at r1 (raw file):
Is it harmless in that IntentCount==LockCount until the cluster version is updated?
Right, until a cluster version is updated and it starts acquiring replicated locks, IntentCount == LockCount. Recall that is just a rename of SeparatedIntentCount, performed by #110590.
I don't know of any reason why IntentCount and SeparatedIntentCount would not be the same after the migration to clear out interleaved intents. I also looked at a collection of debug.zips to sanity check and did not find any discrepancies. Are you aware of any cases where this equality does not hold?
pkg/storage/enginepb/mvcc.proto line 213 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: s/protected/locked?
(in the commit message as well)
I don't think we usually say that the "version is locked", but rather that the "key is locked" and the version is associated with the intent that locks the key. So I think I'll keep "protected" here.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/enginepb/mvcc.go line 137 at r1 (raw file):
Are you aware of any cases where this equality does not hold?
No, not aware of any.
|
TFTRs! bors r+ |
|
Build succeeded: |
Fixes #109645.
Informs #100193.
This commit adds
MVCCStatsfor replicated locks.To do so, it first adds a new field to the stats struct,
LockBytes.LockBytesis the encoded size of replicated locks with shared or exclusive strengths, which are stored in the lock table keyspace. The field includes the size of the locks' keys and their values.For historical reasons, the field excludes the size of intent metadata key-values, even though they are also stored in the lock table keyspace. Intent metadata keys are tracked under KeyBytes and their values are tracked under ValBytes. This is not to be confused with the provisional versioned values protected by the intents, which are tracked by the IntentBytes field (and also by KeyBytes and ValBytes). Hence the vague "without their meta keys" comment above.
The patch then begins accounting for the contributions of replicated locks to
LockBytes,LockCount, andLockAge, of which the second two fields already exist. This accounting is straightforward.The less straightforward part of the patch is MVCC stats computation. Scanning the lock table requires the use of an EngineIterator. To this point, all stats computation has taken place on an MVCCIterator. The patch addresses this by directly scanning the lock table with an EngineIterator (wrapped in a LockTableIterator) during stats computation.
Release note: None