Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use an explicit per-View modification counter [ECR-2804] #658
Use an explicit per-View modification counter [ECR-2804] #658
Changes from 2 commits
82dead4
6e44a6c
bb7a4f1
4bcc1b6
f227820
70f0f9f
80c7ba9
2cad55b
16b6f6d
9c96b2e
206a972
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Seems excessive to me, as we only use it once in the next line. Or maybe make it public? That way it'd be clear for users what the initial value is (because right now in tests to get
initialValue
we callgetCurrentValue
which is error-prone. We could use a public field in tests as well). But I do not insist, it's good as it is as well :)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.
I do not think that the clients shall care about the initial value, what they shall is current value (= the value at the moment when an (spl)iterator is bound to source). initial value is an implementation detail of this counter.
The constnant can probably be inlined, and the field renamed to
counter
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.
A counter of modification events
?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.
Maybe create a task instead of todo? Do you think it's realistic that a counter might overflow
int
range? If yes, we could add throwing an exception innotifyModified
for such a caseThere 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.
The reasoning was to hide the knowledge of how that timestamp is represented to be able to change later, but in an internal interface it seems to be unnecessary.
I've documented, however, that counters must detect up to 4B modifications, that shall be enough :-)
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.
Not sure if a counter shall concern itself with immutable things (on top of that, the AbstractIndexProxy does this check itself)
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.
I think it's better to be safe and throw an exception in case of an immutable counter (as it is now)
This file was deleted.