-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kvserver/observedts: extract package to house logic and docs for observed timestamps #56392
kvserver/observedts: extract package to house logic and docs for observed timestamps #56392
Conversation
44ae17b
to
24803ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like godoc pics up your (great) comments up at all from the var _
declarations:
we need exported symbols for that:
var Foo = roachpb.Transaction{}.MaxTimestamp
which is mildly annoying. It will also generally sort everything, which we also don't necessarily want here.
Maybe for the kind of narrative you have here, what you want is a func Example()
? Or you do the stuff I did in https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1603967685367400?thread_ts=1603967580.366700&cid=C0KB9Q03D, with types and methods on them (sorted too, btw).
Reviewed 7 of 7 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/observedts/doc.go, line 106 at r1 (raw file):
// time higher than the previous leaseholder's clock when it stopped serving // writes. This is accomplished cooperatively for lease transfers and through // a statis period before lease expiration for lease acquisitions. It then
Might this be a good place to mention that we also need to take the start time of the lease into account for shrinking the uncertainty windows? We can never shrink the window past the start of the lease. (LimitTxnMaxTimestamp exists for this reason).
Oh nevermind it's mentioned below.
pkg/roachpb/data.proto, line 363 at r1 (raw file):
// when reading a value in the near future as per the max_timestamp field. // // See pkg/kv/kvserver/observedts for more details.
🙌
24803ee
to
4b0cce4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had kind of given up on godoc because I was struggling to find an approach that I liked for this form of documentation and I wasn't sure how much we cared about packages outside of pkg/kv/kvdoc
, but you're right that we should keep pushing on this and try to find some patterns to center around for these mostly-docs packages.
Exporting the symbols as variables was the most successful approach I found. What do you think of the following?
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 can probably iterate on this a bit more (give a name that sorts to the top more reliably) but no need to do so now, thanks!
Reviewed 5 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)
…rved timestamps This commit introduces a new package which contains logic and documentation related to the observed timestamp system, which allows transactions to track causality between themselves and other, possibly-concurrent, transactions in order to avoid uncertainty related restarts. This ties into our efforts to better document the kv layer. It also ties into synthetic timestamps (cockroachdb#56373), which are important for non-blocking transactions (cockroachdb#52745) and for fixing the interaction between observed timestamps and transaction refreshing (cockroachdb#36431). The proposed fix is to split out a new EffectiveMaxTimestamp field from the MaxTimestamp field on a transaction and set the EffectiveMaxTimestamp in LimitTxnMaxTimestamp. The EffectiveMaxTimestamp field will dictate the uncertainty interval for values with normal timestamps while the original MaxTimestamp will dictate the uncertainty interval for values with synthetic timestamps. In practice, this will mean that observed timestamps do not apply to values with synthetic timestamps. This commit gives us a package to house this new complexity.
4b0cce4
to
58e4c7b
Compare
TFTR! bors r+ |
Build succeeded: |
This commit introduces a new package that contains logic and documentation related to the observed timestamp system, which allows transactions to track causality between themselves and other, possibly-concurrent, transactions in order to avoid uncertainty related restarts.
This ties into our efforts to better document the kv layer.
It also ties into synthetic timestamps (#56373), which are important for non-blocking transactions (#52745) and for fixing the interaction between observed timestamps and transaction refreshing (#36431). The proposed fix is to split out a new EffectiveMaxTimestamp field from the MaxTimestamp field on a transaction and set the EffectiveMaxTimestamp in LimitTxnMaxTimestamp. The EffectiveMaxTimestamp field will dictate the uncertainty interval for values with normal timestamps while the original MaxTimestamp will dictate the uncertainty interval for values with synthetic timestamps. In practice, this will mean that observed timestamps do not apply to values with synthetic timestamps. This commit gives us a package to house this new complexity.