Apache9 commented on a change in pull request #3617:
URL: https://github.com/apache/hbase/pull/3617#discussion_r703328286
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java
##########
@@ -744,14 +749,18 @@ private void deleteDaughterRegions(final
MasterProcedureEnv env) throws IOExcept
throw (InterruptedIOException) new InterruptedIOException().initCause(e);
}
- int daughterA = 0;
- int daughterB = 0;
+ List<Path> daughterA = new ArrayList<>();
+ List<Path> daughterB = new ArrayList<>();
// Look for any exception
for (Future<Pair<Path, Path>> future : futures) {
try {
Pair<Path, Path> p = future.get();
- daughterA += p.getFirst() != null ? 1 : 0;
- daughterB += p.getSecond() != null ? 1 : 0;
+ if(p.getFirst()!=null){
Review comment:
nit: format the code
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java
##########
@@ -691,7 +696,7 @@ private void deleteDaughterRegions(final MasterProcedureEnv
env) throws IOExcept
}
if (nbFiles == 0) {
// no file needs to be splitted.
- return new Pair<Integer, Integer>(0, 0);
+ return new Pair<>(Collections.EMPTY_LIST, Collections.EMPTY_LIST);
Review comment:
nit: Collections.emptyList()
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java
##########
@@ -764,7 +773,7 @@ private void deleteDaughterRegions(final MasterProcedureEnv
env) throws IOExcept
getParentRegion().getShortNameToLog() + " Daughter A: " + daughterA +
" storefiles, Daughter B: " + daughterB + " storefiles.");
}
- return new Pair<Integer, Integer>(daughterA, daughterB);
+ return new Pair<List<Path>, List<Path>>(daughterA, daughterB);
Review comment:
I think new Pair<> is enough? The compiler can infer the actual type.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -17,23 +17,58 @@
*/
package org.apache.hadoop.hbase.regionserver.storefiletracker;
+import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.regionserver.HRegionFileSystem;
import org.apache.hadoop.hbase.regionserver.StoreContext;
+import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.ReflectionUtils;
import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Factory method for creating store file tracker.
*/
@InterfaceAudience.Private
public final class StoreFileTrackerFactory {
-
public static final String TRACK_IMPL = "hbase.store.file-tracker.impl";
+ private static final Logger LOG =
LoggerFactory.getLogger(StoreFileTrackerFactory.class);
public static StoreFileTracker create(Configuration conf, boolean
isPrimaryReplica,
- StoreContext ctx) {
+ StoreContext ctx) {
Class<? extends StoreFileTracker> tracker =
conf.getClass(TRACK_IMPL, DefaultStoreFileTracker.class,
StoreFileTracker.class);
- return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx);
+ try {
+ LOG.info("instantiating StoreFileTracker impl {}", tracker.getName());
+ return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx);
+ } catch (Exception e) {
+ LOG.error("Unable to create StoreFileTracker impl : {}",
tracker.getName(), e);
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static StoreFileTracker create(Configuration conf, boolean
isPrimaryReplica, String family,
+ HRegionFileSystem regionFs) {
+ ColumnFamilyDescriptorBuilder fDescBuilder =
+ ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes(family));
+ StoreContext ctx = StoreContext.getBuilder().
+ withColumnFamilyDescriptor(fDescBuilder.build()).
+ withRegionFileSystem(regionFs).
+ build();
+ return StoreFileTrackerFactory.create(conf, isPrimaryReplica, ctx);
+ }
+
+ public static Configuration mergeConfigurations(Configuration global,
Review comment:
We'd better merge all the configurations? For example, if the tracker
itself needs some configurations...
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java
##########
@@ -48,7 +48,6 @@
*/
@InterfaceAudience.Private
public interface StoreFileTracker {
-
Review comment:
Ditto.
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultStoreEngine.java
##########
@@ -67,6 +67,7 @@ public void testCustomParts() throws Exception {
DummyStoreFlusher.class.getName());
HRegion mockRegion = Mockito.mock(HRegion.class);
HStore mockStore = Mockito.mock(HStore.class);
+ mockStore.conf = conf;
Review comment:
This is a bit strange...
We can not do something like `when(mockStore.getConf()).thenReturn(conf);` ?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/DefaultStoreFileTracker.java
##########
@@ -32,8 +33,7 @@
@InterfaceAudience.Private
class DefaultStoreFileTracker extends StoreFileTrackerBase {
- public DefaultStoreFileTracker(Configuration conf, boolean isPrimaryReplica,
- StoreContext ctx) {
+ public DefaultStoreFileTracker(Configuration conf, boolean isPrimaryReplica,
StoreContext ctx) {
Review comment:
Do not need to touch this file?
--
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.
To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
