HBASE-24382 Flush partial stores of region filtered by seqId when arc… #1737
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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
The change is straight forward but I'm not sure whether it is worth to do this optimization, as it does increase the complexity of code. Have you met the problem introduced by the old simple logic? Thanks. |
Contributor
Author
|
It is from a discussion in our team, about whether a table could be set many column family(10~100) or not. |
Contributor
|
OK, sounds reasonable. On the patch, please just use byte[] as the family name? We use byte[] or String as family name everywhere, especially when passing as a parameter, so using ImmutableByteArray seems a bit strange. |
Contributor
Author
|
Ok, will fix tomorrow. |
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@Apache9 Could you help to review it? Thanks. |
Apache9
requested changes
May 21, 2020
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
So why we choose String instead of byte[] here?
| Collection<HStore> specificStoresToFlush = null; | ||
| if (!forceFlushAllStores && families != null) { | ||
| specificStoresToFlush = stores.entrySet().stream() | ||
| .filter(e -> families.contains(Bytes.toString(e.getKey()))) |
Contributor
There was a problem hiding this comment.
If we want to use contains then we better use a Set instead of a List?
Contributor
Author
There was a problem hiding this comment.
So why we choose String instead of byte[] here?
1、The family name stored as private byte[] in ImmutableByteArray, if use byte[], it need twice convertion.
2、We use the method of contains to filter them later, use string seems esaliy to do.
Correct me if i have some mistake.
If we want to use contains then we better use a Set instead of a List?
Yeah, you are right, will fix.
Thanks a lot.
Contributor
Author
There was a problem hiding this comment.
If the String is ok, i will push a commit later, or else i will research more.
Thanks.
Contributor
There was a problem hiding this comment.
In the WAL interface, we all use byte[] as family name, so I prefer we also use byte[] here. You could use TreeSet with BytesComparator to hold byte[].
Contributor
Author
There was a problem hiding this comment.
Ok, will use byte[] instead of String.
In addition,since we do not need a sorted collection, i could iterate the families to avoid use contains.
Thanks.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
rebase |
Apache9
requested changes
May 23, 2020
| encodedRegionName, r); | ||
| return; | ||
| } | ||
| // force flushing all stores to clean old logs |
| for (Map.Entry<ImmutableByteArray, Long> me : m.entrySet()) { | ||
| if (me.getValue() <= e.getValue()) { | ||
| if (toFlush == null) { | ||
| toFlush = new HashMap<>(); |
Contributor
There was a problem hiding this comment.
Better use a TreeMap with BytesComparator?
|
🎊 +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. |
Contributor
|
FlushPolicy does seem to cut across what you are trying to do here where you only flush a subset.... even though reading the original issue, it seems to be trying to do subset only: https://issues.apache.org/jira/browse/HBASE-10201 We should change flushpolicy to accommodate your need? If we flush all regardless of the policy, that sounds like a problem. Thanks. |
Contributor
Author
|
Skimed HBASE-10201, still can not get the point that how to change the flushpolicy... |
Contributor
Author
|
@saintstack Ping |
Apache9
approved these changes
Jun 23, 2020
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
I'm fine with the approach but I guess there are still checkstyle issues? Please fix them before merging.
Thanks.
| */ | ||
| package org.apache.hadoop.hbase.regionserver; | ||
|
|
||
| import java.util.ArrayList; |
Contributor
There was a problem hiding this comment.
Should remove these imports? Seems we add a method in this class and then moved it out but forgot to remove these imports?
Contributor
Author
There was a problem hiding this comment.
Oh, you are right, fixed.
Thanks.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
bq. The former is about how to select stores that could be flushed, and the latter is about select stores that must be flushed. Ok. Did you stamp this into the code as comments? Let me look. |
Contributor
Author
Will do it later, thanks. |
saintstack
reviewed
Jun 25, 2020
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushRequester.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushRequester.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Show resolved
Hide resolved
| // force flushing all stores to clean old logs | ||
| requester.requestFlush(r, true, FlushLifeCycleTracker.DUMMY); | ||
| // force flushing specified stores to clean old logs | ||
| requester.requestFlush(r, false, families, FlushLifeCycleTracker.DUMMY); |
Contributor
There was a problem hiding this comment.
The flag is set when we are in a critical condition -- the WAL count is in excess of our WAL limit. The flag's intent IIRC is that we flush all stores regardless of what determination is made at flush time as to which stores are in need of flush or not; the old edit may actually be hanging out in a store that is small and not in need of flush normally or in accordance w/ some flush policy. The flag says 'force' the flush. My understanding is that this is a FlushRequest usually but the flag changes the request to a demand.
Here you are passing a set of stores instead where the stores chosen are the ones that will free up WALs. So, this flag is now redundant? We should purge it. Now it will confuse since the 'force' instead is a list of families to flush -- else its null if old behavior.
| // force flushing all stores to clean old logs | ||
| requester.requestFlush(r, true, FlushLifeCycleTracker.DUMMY); | ||
| // force flushing specified stores to clean old logs | ||
| requester.requestFlush(r, false, families, FlushLifeCycleTracker.DUMMY); |
Contributor
There was a problem hiding this comment.
Looking around, this is the only time forceFlushAllStores is set to true -- when we have too many WALs and we need to clear the old ones out.
Contributor
Author
It also set to true in RSRpcServices.flushRegion, would be called by user. |
Contributor
Author
Yeah, i am agree with you, maybe the forceFlushAllStores should be enum, named selectType that include "FORCE_ALL","FORCE_WITH_SPECIFIED","FLUSH_POLICY"? And it is a long story yet, i can do it in follow-on jira, WDYT? |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
bq. It also set to true in RSRpcServices.flushRegion, would be called by user. ok. The user is asking to flush all regions. If user sets it, could convert it to a list of all families? bq. Yeah, i am agree with you, maybe the forceFlushAllStores should be enum, named selectType that include "FORCE_ALL","FORCE_WITH_SPECIFIED","FLUSH_POLICY"? And it is a long story yet, i can do it in follow-on jira, WDYT? Why not just remove it? The above seems to be offering too much choice. Navigating all of the options in flushcache will be awkward. Thanks. |
Contributor
Author
Ok, remove it make sense, let me do it later. |
… how to select stores to flush
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
approved these changes
Jun 26, 2020
| families.addAll(this.getTableDescriptor().getColumnFamilyNames()); | ||
| } | ||
| return this.flushcache(families, writeFlushRequestWalMarker, tracker); | ||
| } |
Contributor
Author
|
Thanks all the comments. |
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.
…hive wal due to too many wals