HBASE-26384 Segment already flushed to hfile may still be remained in… #3777
Conversation
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
… CompactingMemStore
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Seems error not related to this patch, rerun checking |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Oct 26, 2021
| public VersionedSegmentsList getVersionedTail() { | ||
| synchronized (pipeline){ | ||
| List<ImmutableSegment> segmentList = new ArrayList<>(); | ||
| LinkedList<ImmutableSegment> segmentList = new LinkedList<>(); |
There was a problem hiding this comment.
Why change to LinkedList? Usually it will be much slower than ArrayList.
There was a problem hiding this comment.
@Apache9 ， thank you very much for review，Here I change to LinkedList because I expect VersionedSegmentsList.storeSegments is LinkedList， so in the following CompactionPipline.matchAndRemoveSuffixFromPipeline I could use LinkedList.descendingIterator to simplify code.
I think here change to LinkedList is somwhat ok, because:
- Here the returned
VersionedSegmentsListhas only one tail element,LinkedListandArrayListmake no difference. - Furthermore, the
CompactionPipeline.pipelineandCompactionPipeline.readOnlyCopywhich used inCompactionPipeline.getVersionedListare all LinkedList, that is to say,VersionedSegmentsList.storeSegmentsis actually LinkedList in fact(CompactionPipline.getVersionedTailis only used for test), so here change to LinkedList could also make the code looks consistent.
There was a problem hiding this comment.
If ArrayList is not suitable here, is it possible to make use of other array based data structure, like ArrayQueue or ArrayDeque? LinkedList is not recommanded for most cases...
There was a problem hiding this comment.
Ok, thank you for review, I would to try to fix the List problem,
| * This test is for HBASE-26384, | ||
| * test {@link CompactingMemStore#flattenOneSegment} and {@link CompactingMemStore#snapshot()} | ||
| * execute concurrently. | ||
| * The threads sequence is: |
There was a problem hiding this comment.
This is the expected behavior or the broken behavior?
There was a problem hiding this comment.
This is the expected behavior,I modified the comments to explictly state it.
There was a problem hiding this comment.
I prefer we explain the broken behavior first and then say what should be the correct behavior, so it will be easier for people to know what we actually fixed.
There was a problem hiding this comment.
Ok, thank you for suggestion, I would fix the description.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 , thank you for review, I modified the LinkedList problem and test comments problem following your suggestions. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Nov 1, 2021
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java
Show resolved
Hide resolved
Apache9
approved these changes
Nov 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
… CompactingMemStore