saintstack commented on a change in pull request #1774: URL: https://github.com/apache/hbase/pull/1774#discussion_r446446620
########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AbstractAsyncTableRegionLocator.java ########## @@ -0,0 +1,308 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.client; + +import static org.apache.hadoop.hbase.HConstants.EMPTY_END_ROW; +import static org.apache.hadoop.hbase.client.AsyncRegionLocatorHelper.isGood; +import static org.apache.hadoop.hbase.client.ConnectionUtils.createClosestRowAfter; + +import java.io.IOException; +import java.util.HashSet; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import org.apache.commons.lang3.ObjectUtils; +import org.apache.hadoop.hbase.HBaseIOException; +import org.apache.hadoop.hbase.HRegionLocation; +import org.apache.hadoop.hbase.RegionLocations; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting; + +/** + * The base class for locating region of a table. + */ +@InterfaceAudience.Private +abstract class AbstractAsyncTableRegionLocator { + + private static final Logger LOG = LoggerFactory.getLogger(AbstractAsyncTableRegionLocator.class); + + protected final AsyncConnectionImpl conn; + + protected final TableName tableName; + + protected final int maxConcurrent; + + protected final TableRegionLocationCache cache; + + protected static final class LocateRequest { Review comment: Let me resolve. I see that this is just a move of code. My fault for not knowing this code well enough and realizing it not new. ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/CatalogFamilyFormat.java ########## @@ -363,4 +370,27 @@ public static Delete removeRegionReplica(byte[] metaRow, int replicaIndexToDelet } return deleteReplicaLocations; } + + private static byte[] buildLocateRegionStartRow(TableName tableName, byte[] row, + RegionLocateType locateType) { + if (locateType.equals(RegionLocateType.BEFORE)) { Review comment: nit... should be LocateRegionType to match the method names in here. ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/ClientMetaTableAccessor.java ########## @@ -164,26 +164,27 @@ private ClientMetaTableAccessor() { /** * Used to get all region locations for the specific table. - * @param metaTable * @param tableName table we're looking for, can be null for getting all regions * @return the list of region locations. The return value will be wrapped by a * {@link CompletableFuture}. */ public static CompletableFuture<List<HRegionLocation>> getTableHRegionLocations( - AsyncTable<AdvancedScanResultConsumer> metaTable, TableName tableName) { + AsyncTable<AdvancedScanResultConsumer> metaTable, TableName tableName, + boolean excludeOfflinedSplitParents) { CompletableFuture<List<HRegionLocation>> future = new CompletableFuture<>(); - addListener(getTableRegionsAndLocations(metaTable, tableName, true), (locations, err) -> { - if (err != null) { - future.completeExceptionally(err); - } else if (locations == null || locations.isEmpty()) { - future.complete(Collections.emptyList()); - } else { - List<HRegionLocation> regionLocations = - locations.stream().map(loc -> new HRegionLocation(loc.getFirst(), loc.getSecond())) - .collect(Collectors.toList()); - future.complete(regionLocations); - } - }); + addListener(getTableRegionsAndLocations(metaTable, tableName, excludeOfflinedSplitParents), Review comment: Trying to understand why we need to support this flag excludeOfflinedSplitParents Snapshots need it? Are we trying to support snapshotting hbase:meta table? I was going to suggest we not do this but thinking on it, it is probably a useful utility to have. ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AbstractAsyncTableRegionLocator.java ########## @@ -0,0 +1,308 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.client; + +import static org.apache.hadoop.hbase.HConstants.EMPTY_END_ROW; +import static org.apache.hadoop.hbase.client.AsyncRegionLocatorHelper.isGood; +import static org.apache.hadoop.hbase.client.ConnectionUtils.createClosestRowAfter; + +import java.io.IOException; +import java.util.HashSet; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import org.apache.commons.lang3.ObjectUtils; +import org.apache.hadoop.hbase.HBaseIOException; +import org.apache.hadoop.hbase.HRegionLocation; +import org.apache.hadoop.hbase.RegionLocations; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting; + +/** + * The base class for locating region of a table. + */ +@InterfaceAudience.Private +abstract class AbstractAsyncTableRegionLocator { Review comment: Why TableRegion rather than Region? The 'Table' is redundant and makes this class name a mouthful? ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AbstractAsyncTableRegionLocator.java ########## @@ -0,0 +1,308 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.client; + +import static org.apache.hadoop.hbase.HConstants.EMPTY_END_ROW; +import static org.apache.hadoop.hbase.client.AsyncRegionLocatorHelper.isGood; +import static org.apache.hadoop.hbase.client.ConnectionUtils.createClosestRowAfter; + +import java.io.IOException; +import java.util.HashSet; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import org.apache.commons.lang3.ObjectUtils; +import org.apache.hadoop.hbase.HBaseIOException; +import org.apache.hadoop.hbase.HRegionLocation; +import org.apache.hadoop.hbase.RegionLocations; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting; + +/** + * The base class for locating region of a table. + */ +@InterfaceAudience.Private +abstract class AbstractAsyncTableRegionLocator { Review comment: I see you are doing this.... delete mode 100644 hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonMetaRegionLocator.java create mode 100644 hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonMetaTableRegionLocator.java .... you want to create new classes rather than just replace the old and the TableRegion is how you distingush the new set? ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocator.java ########## @@ -85,26 +118,41 @@ private boolean isMeta(TableName tableName) { return TableName.isMetaTableName(tableName); } + private AbstractAsyncTableRegionLocator getOrCreateTableRegionLocator(TableName tableName) { Review comment: Can't return an Interface rather than Abstract class? ---------------------------------------------------------------- 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
