Refactor[mqbs::FileStore]: Rename d_primaryLeaseId to d_writeHeadLeaseId - #1660
Conversation
| /// value, not the cluster-state primary lease id; the two can differ | ||
| /// transiently (e.g. during recovery/conform, or on a replica lagging the | ||
| /// primary). | ||
| unsigned int d_writeHeadLeaseId; |
There was a problem hiding this comment.
In reality it's the write-head lease id — the lease id stamped on the next record this store writes or applies, paired with the sequence number d_highestSeqNums[d_writeHeadLeaseId].
If this is the next lease id, why not naming it d_nextWriteLeaseId?
I think that the PR's proposed name is less obvious than this.
Let's also update the docs to explicitly say if the currently stored lease id is written already or is it intended for the next write.
| /// Lease id of the current primary. The current sequence number is | ||
| /// always `d_highestSeqNums[d_primaryLeaseId]`. | ||
| unsigned int d_primaryLeaseId; | ||
| /// Lease id stamped on the next record this store writes or applies (the |
There was a problem hiding this comment.
stamped - personally I don't like the complete form of action, if it is not stamped yet.
There was a problem hiding this comment.
I'd push back on d_nextWriteLeaseId. Its partner sequenceNumber() returns the highest/last seq already written under this lease, not the next — and the pair (d_writeHeadLeaseId, sequenceNumber()) is logged everywhere as the current/self PSN, i.e. the position already reached. Naming the lease "next" while its seq half is "last" makes the two halves of the same PSN contradict each other.
"Write head" (a cursor) fits both: it sits at the last written position and is where the next write lands. "next" only names half of that.
| /// "write head"; current sequence number is | ||
| /// `d_highestSeqNums[d_writeHeadLeaseId]`). This is a storage-layer | ||
| /// value, not the cluster-state primary lease id; the two can differ | ||
| /// transiently (e.g. during recovery/conform, or on a replica lagging the |
There was a problem hiding this comment.
not the cluster-state primary lease id; the two can differ
/// transiently (e.g. during recovery/conform, or on a replica lagging the
A general comment, I think the docs should describe what something is, not what something isn't
Signed-off-by: Yuan Jing Vincent Yan <yyan82@bloomberg.net>
d676d66 to
b349510
Compare
Summary
mqbs::FileStore::d_primaryLeaseIdtod_writeHeadLeaseIdto reflect what the member actually is.d_highestSeqNums[d_writeHeadLeaseId].setActivePrimary); it can differ transiently from the cluster's primary lease id (e.g. during recovery, or on a replica lagging the primary). The clearer name prevents that misreading.