[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r445323556
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2400,6 +2405,7 @@ public FlushResult flush(boolean force) throws
IOException {
* This method may block for some time, so it should not be called from a
* time-sensitive thread.
* @param forceFlushAllStores whether we want to flush all stores
+ * @param families stores of region to flush.
Review comment:
No, only if forceFlushAllStores is true we select all families.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737: URL: https://github.com/apache/hbase/pull/1737#discussion_r444302902 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushPolicy.java ## @@ -17,7 +17,10 @@ */ package org.apache.hadoop.hbase.regionserver; +import java.util.ArrayList; Review comment: Oh, you are right, fixed. Thanks. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r438074445
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java
##
@@ -58,8 +60,8 @@ protected void scheduleFlush(String encodedRegionName) {
encodedRegionName, r);
return;
}
-// 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);
Review comment:
Errr, is this line could be executed in other case except too many wals?
Checked the code again, seems not, Correct me if i miss something.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737: URL: https://github.com/apache/hbase/pull/1737#discussion_r436492980 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushRequester.java ## @@ -36,6 +38,18 @@ */ boolean requestFlush(HRegion region, boolean forceFlushAllStores, FlushLifeCycleTracker tracker); + /** + * Tell the listener the cache needs to be flushed. + * + * @param region the Region requesting the cache flush + * @param forceFlushAllStores whether we want to flush all stores. e.g., when request from log Review comment: Will add some content for this. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r436491431
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushPolicy.java
##
@@ -46,4 +49,16 @@ protected void configureForRegion(HRegion region) {
*/
public abstract Collection selectStoresToFlush();
+ /**
+ * select stores which matches the specified families
+ *
+ * @return the stores need to be flushed.
+ */
+ public Collection selectStoresToFlush(Map stores,
List families) {
Review comment:
Yeah, if still as part of FlushPolicy after discuss.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r436490904
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java
##
@@ -58,8 +60,8 @@ protected void scheduleFlush(String encodedRegionName) {
encodedRegionName, r);
return;
}
-// 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);
Review comment:
Yeah, it changes and only changes behavior in case that wals is too many.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r436490171
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##
@@ -180,18 +181,18 @@ public void run() {
WAL wal = entry.getKey();
// reset the flag in front to avoid missing roll request before we
return from rollWriter.
walNeedsRoll.put(wal, Boolean.FALSE);
- byte[][] regionsToFlush = null;
+ Map> regionsToFlush = null;
try {
// Force the roll if the logroll.period is elapsed or if a roll
was requested.
-// The returned value is an array of actual region names.
+// The returned value is an collection of actual region and family
names.
regionsToFlush = wal.rollWriter(periodic ||
entry.getValue().booleanValue());
} catch (WALClosedException e) {
LOG.warn("WAL has been closed. Skipping rolling of writer and just
remove it", e);
iter.remove();
}
if (regionsToFlush != null) {
-for (byte[] r : regionsToFlush) {
- scheduleFlush(Bytes.toString(r));
+for (Map.Entry> r :
regionsToFlush.entrySet()) {
Review comment:
I understand your concern, but i think the test is enough, and we just
change the behavier when families is not null, and it will only comes from
logRoller, and it only could be happend when wals is too many.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r436484665
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2457,8 +2463,13 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = flushPolicy.selectStoresToFlush(stores,
families);
Review comment:
> Why do we pass in stores into selectStoresToFlush? We do not need the
list of stores in the flushAllStoresPolicy. It has access to 'this' so can
figure what Stores are in the Region.
As comment below, maybe it should not be part of flushPolicy?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737: URL: https://github.com/apache/hbase/pull/1737#discussion_r433123740 ## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/AbstractTestWALReplay.java ## @@ -9,7 +9,7 @@ * with the License. You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 - * + *TestFlusher Review comment: Will fix, editing error. Thanks. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r433122752
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
##
@@ -639,26 +641,30 @@ public int getNumLogFiles() {
* If the number of un-archived WAL files ('live' WALs) is greater than
maximum allowed,
* check the first (oldest) WAL, and return those regions which should be
flushed so that
* it can be let-go/'archived'.
- * @return regions (encodedRegionNames) to flush in order to archive oldest
WAL file.
+ * @return stores of regions (encodedRegionNames) to flush in order to
archive oldest WAL file.
*/
- byte[][] findRegionsToForceFlush() throws IOException {
-byte[][] regions = null;
+ Map> findRegionsToForceFlush() throws IOException {
+Map> regions = null;
int logCount = getNumRolledLogFiles();
if (logCount > this.maxLogs && logCount > 0) {
Map.Entry firstWALEntry =
this.walFile2Props.firstEntry();
regions =
this.sequenceIdAccounting.findLower(firstWALEntry.getValue().encodedName2HighestSequenceId);
}
if (regions != null) {
- StringBuilder sb = new StringBuilder();
- for (int i = 0; i < regions.length; i++) {
-if (i > 0) {
- sb.append(", ");
+ List listForPrint = new ArrayList();
+ for (Map.Entry> r : regions.entrySet()) {
+StringBuffer families = new StringBuffer();
Review comment:
Will fix.
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceIdAccounting.java
##
@@ -19,13 +19,7 @@
import static org.apache.hadoop.hbase.util.ConcurrentMapUtils.computeIfAbsent;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;
Review comment:
Will fix.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r433122611
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceIdAccounting.java
##
@@ -440,27 +434,27 @@ boolean areAllLower(Map sequenceids,
Collection keysBlocki
* {@link #lowestUnflushedSequenceIds} has a sequence id less than that
passed in
* sequenceids then return it.
* @param sequenceids Sequenceids keyed by encoded region name.
- * @return regions found in this instance with sequence ids less than those
passed in.
+ * @return stores of regions found in this instance with sequence ids less
than those passed in.
*/
- byte[][] findLower(Map sequenceids) {
-List toFlush = null;
+ Map> findLower(Map sequenceids) {
+Map> toFlush = null;
// Keeping the old behavior of iterating unflushedSeqNums under
oldestSeqNumsLock.
synchronized (tieLock) {
for (Map.Entry e : sequenceids.entrySet()) {
Map m =
this.lowestUnflushedSequenceIds.get(e.getKey());
if (m == null) {
continue;
}
-// The lowest sequence id outstanding for this region.
-long lowest = getLowestSequenceId(m);
-if (lowest != HConstants.NO_SEQNUM && lowest <= e.getValue()) {
- if (toFlush == null) {
-toFlush = new ArrayList<>();
+for (Map.Entry me : m.entrySet()) {
+ if (me.getValue() <= e.getValue()) {
+if (toFlush == null) {
+ toFlush = new TreeMap(Bytes.BYTES_COMPARATOR);
+}
+toFlush.computeIfAbsent(e.getKey(), k -> new
ArrayList<>()).add(Bytes.toBytes(me.getKey().toString()));
}
- toFlush.add(e.getKey());
}
}
}
-return toFlush == null ? null : toFlush.toArray(new byte[0][]);
+return toFlush == null ? null : toFlush;
Review comment:
Oh, awkward.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r432271964
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = new ArrayList<>();
+ for (byte[] family : families) {
+specificStoresToFlush.add(stores.get(family));
Review comment:
Moved to FlushPolicy, seems better.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r432269012
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
##
@@ -639,26 +641,30 @@ public int getNumLogFiles() {
* If the number of un-archived WAL files ('live' WALs) is greater than
maximum allowed,
* check the first (oldest) WAL, and return those regions which should be
flushed so that
* it can be let-go/'archived'.
- * @return regions (encodedRegionNames) to flush in order to archive oldest
WAL file.
+ * @return stores of regions (encodedRegionNames) to flush in order to
archive oldest WAL file.
*/
- byte[][] findRegionsToForceFlush() throws IOException {
-byte[][] regions = null;
+ Map> findRegionsToForceFlush() throws IOException {
+Map> regions = null;
int logCount = getNumRolledLogFiles();
if (logCount > this.maxLogs && logCount > 0) {
Map.Entry firstWALEntry =
this.walFile2Props.firstEntry();
regions =
this.sequenceIdAccounting.findLower(firstWALEntry.getValue().encodedName2HighestSequenceId);
}
if (regions != null) {
- StringBuilder sb = new StringBuilder();
- for (int i = 0; i < regions.length; i++) {
-if (i > 0) {
- sb.append(", ");
+ List listForPrint = new ArrayList();
+ for (Map.Entry> r : regions.entrySet()) {
+StringBuffer families = new StringBuffer();
+for (int i = 0; i < r.getValue().size(); i++) {
+ if (i > 0) {
+families.append("|");
+ }
+ families.append(Bytes.toString(r.getValue().get(i)));
}
-sb.append(Bytes.toStringBinary(regions[i]));
+listForPrint.add(Bytes.toStringBinary(r.getKey()) + "[" +
families.toString() + "]");
}
LOG.info("Too many WALs; count=" + logCount + ", max=" + this.maxLogs +
-"; forcing flush of " + regions.length + " regions(s): " +
sb.toString());
+"; forcing flush partial stores of " + regions.size() + " region(s): "
+ StringUtils.join(",", listForPrint));
Review comment:
Will fix.
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
##
@@ -639,26 +641,30 @@ public int getNumLogFiles() {
* If the number of un-archived WAL files ('live' WALs) is greater than
maximum allowed,
* check the first (oldest) WAL, and return those regions which should be
flushed so that
* it can be let-go/'archived'.
- * @return regions (encodedRegionNames) to flush in order to archive oldest
WAL file.
+ * @return stores of regions (encodedRegionNames) to flush in order to
archive oldest WAL file.
*/
- byte[][] findRegionsToForceFlush() throws IOException {
-byte[][] regions = null;
+ Map> findRegionsToForceFlush() throws IOException {
+Map> regions = null;
int logCount = getNumRolledLogFiles();
if (logCount > this.maxLogs && logCount > 0) {
Map.Entry firstWALEntry =
this.walFile2Props.firstEntry();
regions =
this.sequenceIdAccounting.findLower(firstWALEntry.getValue().encodedName2HighestSequenceId);
}
if (regions != null) {
- StringBuilder sb = new StringBuilder();
- for (int i = 0; i < regions.length; i++) {
-if (i > 0) {
- sb.append(", ");
+ List listForPrint = new ArrayList();
+ for (Map.Entry> r : regions.entrySet()) {
+StringBuffer families = new StringBuffer();
+for (int i = 0; i < r.getValue().size(); i++) {
+ if (i > 0) {
+families.append("|");
Review comment:
Will fix
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r432268692
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
##
@@ -639,26 +641,30 @@ public int getNumLogFiles() {
* If the number of un-archived WAL files ('live' WALs) is greater than
maximum allowed,
* check the first (oldest) WAL, and return those regions which should be
flushed so that
* it can be let-go/'archived'.
- * @return regions (encodedRegionNames) to flush in order to archive oldest
WAL file.
+ * @return stores of regions (encodedRegionNames) to flush in order to
archive oldest WAL file.
*/
- byte[][] findRegionsToForceFlush() throws IOException {
-byte[][] regions = null;
+ Map> findRegionsToForceFlush() throws IOException {
+Map> regions = null;
int logCount = getNumRolledLogFiles();
if (logCount > this.maxLogs && logCount > 0) {
Map.Entry firstWALEntry =
this.walFile2Props.firstEntry();
regions =
this.sequenceIdAccounting.findLower(firstWALEntry.getValue().encodedName2HighestSequenceId);
}
if (regions != null) {
- StringBuilder sb = new StringBuilder();
- for (int i = 0; i < regions.length; i++) {
-if (i > 0) {
- sb.append(", ");
+ List listForPrint = new ArrayList();
+ for (Map.Entry> r : regions.entrySet()) {
+StringBuffer families = new StringBuffer();
+for (int i = 0; i < r.getValue().size(); i++) {
+ if (i > 0) {
+families.append("|");
Review comment:
will fix
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r432267965
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java
##
@@ -459,13 +459,18 @@ private FlushType isAboveLowWaterMark() {
}
@Override
- public boolean requestFlush(HRegion r, boolean forceFlushAllStores,
- FlushLifeCycleTracker tracker) {
+ public boolean requestFlush(HRegion r, boolean
forceFlushAllStores,FlushLifeCycleTracker tracker) {
Review comment:
Ditto
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r432267596
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = new ArrayList<>();
+ for (byte[] family : families) {
+specificStoresToFlush.add(stores.get(family));
+ }
+}else{
Review comment:
Sorry for the careless.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r429540273
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java
##
@@ -59,7 +61,7 @@ protected void scheduleFlush(String encodedRegionName) {
return;
}
// force flushing all stores to clean old logs
Review comment:
fixed
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r429540319
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceIdAccounting.java
##
@@ -440,27 +440,27 @@ boolean areAllLower(Map sequenceids,
Collection keysBlocki
* {@link #lowestUnflushedSequenceIds} has a sequence id less than that
passed in
* sequenceids then return it.
* @param sequenceids Sequenceids keyed by encoded region name.
- * @return regions found in this instance with sequence ids less than those
passed in.
+ * @return stores of regions found in this instance with sequence ids less
than those passed in.
*/
- byte[][] findLower(Map sequenceids) {
-List toFlush = null;
+ Map> findLower(Map sequenceids) {
+Map> toFlush = null;
// Keeping the old behavior of iterating unflushedSeqNums under
oldestSeqNumsLock.
synchronized (tieLock) {
for (Map.Entry e : sequenceids.entrySet()) {
Map m =
this.lowestUnflushedSequenceIds.get(e.getKey());
if (m == null) {
continue;
}
-// The lowest sequence id outstanding for this region.
-long lowest = getLowestSequenceId(m);
-if (lowest != HConstants.NO_SEQNUM && lowest <= e.getValue()) {
- if (toFlush == null) {
-toFlush = new ArrayList<>();
+for (Map.Entry me : m.entrySet()) {
+ if (me.getValue() <= e.getValue()) {
+if (toFlush == null) {
+ toFlush = new HashMap<>();
Review comment:
Fixed.
Thanks a lot.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r429540273
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java
##
@@ -59,7 +61,7 @@ protected void scheduleFlush(String encodedRegionName) {
return;
}
// force flushing all stores to clean old logs
Review comment:
Fixed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r428468856
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = stores.entrySet().stream()
+.filter(e -> families.contains(Bytes.toString(e.getKey(
Review 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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r428468856
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = stores.entrySet().stream()
+.filter(e -> families.contains(Bytes.toString(e.getKey(
Review comment:
Ok, will use byte[] instead of String.
In addition,since we does not need a sorted collection, i could iterate the
families to avoid use contains.
Thanks.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r428419937
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = stores.entrySet().stream()
+.filter(e -> families.contains(Bytes.toString(e.getKey(
Review comment:
If the String is ok, i will push a commit later, or else i will research
more.
Thanks.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r428412426
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = stores.entrySet().stream()
+.filter(e -> families.contains(Bytes.toString(e.getKey(
Review 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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r428412426
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = stores.entrySet().stream()
+.filter(e -> families.contains(Bytes.toString(e.getKey(
Review 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, wil fix.
Thanks a lot.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r428412426
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = stores.entrySet().stream()
+.filter(e -> families.contains(Bytes.toString(e.getKey(
Review 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, wil fix.
Thanks a lot.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #1737: HBASE-24382 Flush partial stores of region filtered by seqId when arc…
bsglz commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r428412426
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -2444,8 +2451,16 @@ public FlushResultImpl flushcache(boolean
forceFlushAllStores, boolean writeFlus
}
try {
-Collection specificStoresToFlush =
+Collection specificStoresToFlush = null;
+if (!forceFlushAllStores && families != null) {
+ specificStoresToFlush = stores.entrySet().stream()
+.filter(e -> families.contains(Bytes.toString(e.getKey(
Review 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, wil fix.
Thanks a lot.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
