Add contains method to LocalCheckpointTracker#33871
Merged
dnhatn merged 3 commits intoelastic:masterfrom Sep 20, 2018
Merged
Conversation
This change adds "contains" method to LocalCheckpointTracker. One of the use cases is to check if a given operation has been processed in an engine or not by looking up its seq_no in LocalCheckpointTracker.
dnhatn
added
>enhancement
v7.0.0
:Distributed/Distributed
jasontedor
reviewed
Sep 19, 2018
| /** | ||
| * Checks if the given sequence number was marked as completed in this tracker. | ||
| */ | ||
| public synchronized boolean contains(long seqNo) { |
Member
There was a problem hiding this comment.
I don't think we need to be fully synchronized here (and so can save ourselves from locking for the quick returns). I think that we only need to lock on this on access to processedSeqNo. I see that we have asserts that we have a lock on this in getBitSetKey and seqNoToBitSetOffset but I think that neither of those are necessary.
jasontedor
approved these changes
Sep 19, 2018
Member
jasontedor
left a comment
jasontedor
left a comment
There was a problem hiding this comment.
Looks good @dnhatn! Thanks.
Member
Author
|
Thanks @s1monw and @jasontedor. |
dnhatn
added a commit
that referenced
this pull request
Sep 20, 2018
This change adds "contains" method to LocalCheckpointTracker. One of the use cases is to check if a given operation has been processed in an engine or not by looking up its seq_no in LocalCheckpointTracker. Relates #33656
dnhatn
added a commit
that referenced
this pull request
Sep 20, 2018
It's possible for the set "seqNos" to contain only the "unFinishedSeq" in the testConcurrentReplica test. If this is the case, the call `randomValueOtherThan` won't make any progress because the predicate will never be false. This commit removes this expectation because it's incorrect and it's no longer needed as we have a dedicated test to verify the contains method. Relates #33871
dnhatn
added a commit
that referenced
this pull request
Sep 20, 2018
It's possible for the set "seqNos" to contain only the "unFinishedSeq" in the testConcurrentReplica test. If this is the case, the call `randomValueOtherThan` won't make any progress because the predicate will never be false. This commit removes this expectation because it's incorrect and it's no longer needed as we have a dedicated test to verify the contains method. Relates #33871
kcm
pushed a commit
that referenced
this pull request
Oct 30, 2018
This change adds "contains" method to LocalCheckpointTracker. One of the use cases is to check if a given operation has been processed in an engine or not by looking up its seq_no in LocalCheckpointTracker. Relates #33656
kcm
pushed a commit
that referenced
this pull request
Oct 30, 2018
It's possible for the set "seqNos" to contain only the "unFinishedSeq" in the testConcurrentReplica test. If this is the case, the call `randomValueOtherThan` won't make any progress because the predicate will never be false. This commit removes this expectation because it's incorrect and it's no longer needed as we have a dedicated test to verify the contains method. Relates #33871
dnhatn
added a commit
that referenced
this pull request
Feb 12, 2019
We are accessing the `CountedBitSet` in `LocalCheckpointTracker#contains` without proper synchronization. Relates #33871
dnhatn
added a commit
that referenced
this pull request
Feb 12, 2019
We are accessing the `CountedBitSet` in `LocalCheckpointTracker#contains` without proper synchronization. Relates #33871
dnhatn
added a commit
that referenced
this pull request
Feb 12, 2019
We are accessing the `CountedBitSet` in `LocalCheckpointTracker#contains` without proper synchronization. Relates #33871
dnhatn
added a commit
that referenced
this pull request
Feb 12, 2019
We are accessing the `CountedBitSet` in `LocalCheckpointTracker#contains` without proper synchronization. Relates #33871
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This change adds "contains" method to LocalCheckpointTracker.
One of the use cases is to check if a given operation has been processed
in an engine or not by looking up its seq_no in LocalCheckpointTracker.
Relates #33656