storage: clarify consistency as of when #158165
Conversation
This commit clarifies in Engine.NewReader that consistent iterators are not that as of the NewReader() call time. This is already mentioned in the Reader interface comment, but it is easy to miss in the details. Worth calling it out in the NewReader comment directly because this is the user's entry point into "reading" and it proved easy to assume that this is where consistency happens. Epic: none Release note: none
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
| // | ||
| // NB: consistent iterators are not as of when NewReader is called. They are | ||
| // as of the time when the first iterator is created (often, this is when the | ||
| // first read happens if iterators are not used directly), or earlier if |
There was a problem hiding this comment.
Looking for suggestion on how to word this. Iterators are always used, but I mean to say that they are often hidden behind things (e.g. MVCCGet), so it can be the "first read/access" from the storage package user's perspective.
There was a problem hiding this comment.
I think we can leave it as the simple "when the first iterator is created, or earlier is Reader.PinEngineStateForIterators ...".
Code readers are expected to know that passing a Reader to some function could cause it to create an iterator.
| // NB: consistent iterators are not as of when NewReader is called. They are | ||
| // as of the time when the first iterator is created (often, this is when the | ||
| // first read happens if iterators are not used directly), or earlier if | ||
| // Reader.PinEngineStateForIterators is called. | ||
| NewReader(durability DurabilityRequirement) Reader |
There was a problem hiding this comment.
Any other places to call this out? NewReadOnly below seems like one.
There was a problem hiding this comment.
We have many methods that give us a Reader: NewBatch, NewReader, NewReadOnly, NewUnindexedBatch.
We can either repeat the earlier comment with each. Or move that comment to the Reader declaration and point to it from all the methods. I have a preference for the latter.
sumeerbhola
left a comment
sumeerbhola
left a comment
There was a problem hiding this comment.
@sumeerbhola reviewed all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @pav-kv)
| // | ||
| // NB: consistent iterators are not as of when NewReader is called. They are | ||
| // as of the time when the first iterator is created (often, this is when the | ||
| // first read happens if iterators are not used directly), or earlier if |
There was a problem hiding this comment.
I think we can leave it as the simple "when the first iterator is created, or earlier is Reader.PinEngineStateForIterators ...".
Code readers are expected to know that passing a Reader to some function could cause it to create an iterator.
| // NB: consistent iterators are not as of when NewReader is called. They are | ||
| // as of the time when the first iterator is created (often, this is when the | ||
| // first read happens if iterators are not used directly), or earlier if | ||
| // Reader.PinEngineStateForIterators is called. | ||
| NewReader(durability DurabilityRequirement) Reader |
There was a problem hiding this comment.
We have many methods that give us a Reader: NewBatch, NewReader, NewReadOnly, NewUnindexedBatch.
We can either repeat the earlier comment with each. Or move that comment to the Reader declaration and point to it from all the methods. I have a preference for the latter.
3 participants
This commit clarifies in
Engine.NewReaderthat consistent iterators are not that as of theNewReader()call time.This is already mentioned in the
Readerinterface comment, but it is easy to miss in the details. Worth calling it out in theNewReadercomment directly because this is the user's entry point into "reading" and it proved easy to assume that this is where consistency happens.Epic: none
Release note: none