saintstack commented on a change in pull request #2584:
URL: https://github.com/apache/hbase/pull/2584#discussion_r514543049
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -182,8 +183,20 @@ public void newDead(ServerName sn) {
}
private void spawnRenewalChore(final UserGroupInformation user) {
- authService = new ChoreService("Relogin service");
- authService.scheduleChore(AuthUtil.getAuthRenewalChore(user));
+ ChoreService service = getChoreService();
+ service.scheduleChore(AuthUtil.getAuthRenewalChore(user));
+ }
+
+ /**
+ * If choreService has not been created yet, create the ChoreService.
+ * It is not thread safe.
+ * @return ChoreService
+ */
+ ChoreService getChoreService() {
Review comment:
Do we need this?
It is called from the constructor only. If so, just create choreservice in
the constructor.
If getChoreService can be called from elsewhere, then does this need to be
synchronized so we don't make multiple chore service instances?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonMetaRegionLocator.java
##########
@@ -196,8 +202,43 @@ private boolean tryComplete(LocateRequest req,
CompletableFuture<RegionLocations
MAX_CONCURRENT_LOCATE_REQUEST_PER_TABLE,
DEFAULT_MAX_CONCURRENT_LOCATE_REQUEST_PER_TABLE);
this.locatePrefetchLimit =
conn.getConfiguration().getInt(LOCATE_PREFETCH_LIMIT,
DEFAULT_LOCATE_PREFETCH_LIMIT);
- this.useMetaReplicas =
- conn.getConfiguration().getBoolean(USE_META_REPLICAS,
DEFAULT_USE_META_REPLICAS);
+
+ // Get the region locator's meta replica mode.
+ this.metaReplicaMode = CatalogReplicaMode.valueOf(conn.getConfiguration()
+ .get(LOCATOR_META_REPLICAS_MODE, CatalogReplicaMode.None.toString()));
+
+ switch (this.metaReplicaMode) {
+ case LoadBalance:
+ String replicaSelectorClass = conn.getConfiguration().
+ get(RegionLocator.LOCATOR_META_REPLICAS_MODE_LOADBALANCE_SELECTOR,
+ CatalogReplicaLoadBalanceSimpleSelector.class.getName());
+
+ this.metaReplicaSelector =
CatalogReplicaLoadBalanceSelectorFactory.createSelector(
+ replicaSelectorClass, META_TABLE_NAME, conn, () -> {
+ int numOfReplicas = 1;
+ try {
+ RegionLocations metaLocations =
conn.registry.getMetaRegionLocations().get(
+ GET_META_LOCATIONS_TIMEOUT, TimeUnit.MILLISECONDS);
+ numOfReplicas = metaLocations.size();
+ } catch (Exception e) {
+ LOG.error("Failed to get table {}'s region replication, ",
META_TABLE_NAME, e);
+ }
+ return numOfReplicas;
+ });
+ break;
+ case None:
+ // If user does not configure LOCATOR_META_REPLICAS_MODE, let's check
the legacy config.
+ if (this.metaReplicaMode == CatalogReplicaMode.None) {
+ boolean useMetaReplicas =
conn.getConfiguration().getBoolean(USE_META_REPLICAS,
+ DEFAULT_USE_META_REPLICAS);
+ if (useMetaReplicas) {
+ this.metaReplicaMode = CatalogReplicaMode.HedgedRead;
Review comment:
None is not the same as hedged read, right?
If user configures hedged read as metaReplicaMode, they fall through to
default which 'does nothing'. That don't seem right. Shouldn't it have a case
here?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionLocator.java
##########
@@ -37,6 +37,12 @@
*/
@InterfaceAudience.Public
public interface RegionLocator extends Closeable {
+
+ String LOCATOR_META_REPLICAS_MODE = "hbase.locator.meta.replicas.mode";
Review comment:
Needs javadoc to at least point at what these are about.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonMetaRegionLocator.java
##########
@@ -433,9 +474,24 @@ private void locateInMeta(TableName tableName,
LocateRequest req) {
Scan scan = new Scan().withStartRow(metaStartKey).withStopRow(metaStopKey,
true)
.addFamily(HConstants.CATALOG_FAMILY).setReversed(true).setCaching(locatePrefetchLimit)
.setReadType(ReadType.PREAD);
- if (useMetaReplicas) {
- scan.setConsistency(Consistency.TIMELINE);
+
+ switch (this.metaReplicaMode) {
+ case LoadBalance:
+ int metaReplicaId = this.metaReplicaSelector.select(tableName,
req.row, req.locateType);
+ if (metaReplicaId != RegionInfo.DEFAULT_REPLICA_ID) {
+ // If the selector gives a non-primary meta replica region, then go
with it.
+ // Otherwise, just go to primary in non-hedgedRead mode.
+ scan.setConsistency(Consistency.TIMELINE);
+ scan.setReplicaId(metaReplicaId);
+ }
+ break;
+ case HedgedRead:
+ scan.setConsistency(Consistency.TIMELINE);
+ break;
+ default:
+ // do nothing
Review comment:
Yeah, said this before but in follow-on, would be good to shove all this
stuff into a CatalogReplicaMode class. Internally this class would figure which
policy to run. It would have a method that took a Scan that allowed decorating
the Scan w/ whatever the mode needed to implement its policy. Later.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonMetaRegionLocator.java
##########
@@ -577,6 +635,15 @@ private void removeLocationFromCache(HRegionLocation loc) {
if (!canUpdateOnError(loc, oldLoc)) {
return;
}
+ // Tell metaReplicaSelector that the location is stale. It will create a
stale entry
+ // with timestamp internally. Next time the client looks up the same
location,
+ // it will pick a different meta replica region. For the current
implementation,
+ // the metaReplicaId is not used, so the primary one is passed in.
+ if (this.metaReplicaMode == CatalogReplicaMode.LoadBalance) {
+ // TODO: pass in -1 as currently fromReplicaId is not being used.
Review comment:
Comment above doesn't agree w/ this one? Comment above says that we are
passing the primary? (Agree that passing -1 is right here..)
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonMetaRegionLocator.java
##########
@@ -196,8 +202,43 @@ private boolean tryComplete(LocateRequest req,
CompletableFuture<RegionLocations
MAX_CONCURRENT_LOCATE_REQUEST_PER_TABLE,
DEFAULT_MAX_CONCURRENT_LOCATE_REQUEST_PER_TABLE);
this.locatePrefetchLimit =
conn.getConfiguration().getInt(LOCATE_PREFETCH_LIMIT,
DEFAULT_LOCATE_PREFETCH_LIMIT);
- this.useMetaReplicas =
- conn.getConfiguration().getBoolean(USE_META_REPLICAS,
DEFAULT_USE_META_REPLICAS);
+
+ // Get the region locator's meta replica mode.
+ this.metaReplicaMode = CatalogReplicaMode.valueOf(conn.getConfiguration()
+ .get(LOCATOR_META_REPLICAS_MODE, CatalogReplicaMode.None.toString()));
+
+ switch (this.metaReplicaMode) {
+ case LoadBalance:
+ String replicaSelectorClass = conn.getConfiguration().
+ get(RegionLocator.LOCATOR_META_REPLICAS_MODE_LOADBALANCE_SELECTOR,
+ CatalogReplicaLoadBalanceSimpleSelector.class.getName());
+
+ this.metaReplicaSelector =
CatalogReplicaLoadBalanceSelectorFactory.createSelector(
+ replicaSelectorClass, META_TABLE_NAME, conn, () -> {
+ int numOfReplicas = 1;
+ try {
+ RegionLocations metaLocations =
conn.registry.getMetaRegionLocations().get(
+ GET_META_LOCATIONS_TIMEOUT, TimeUnit.MILLISECONDS);
+ numOfReplicas = metaLocations.size();
+ } catch (Exception e) {
+ LOG.error("Failed to get table {}'s region replication, ",
META_TABLE_NAME, e);
+ }
+ return numOfReplicas;
+ });
+ break;
+ case None:
+ // If user does not configure LOCATOR_META_REPLICAS_MODE, let's check
the legacy config.
+ if (this.metaReplicaMode == CatalogReplicaMode.None) {
+ boolean useMetaReplicas =
conn.getConfiguration().getBoolean(USE_META_REPLICAS,
+ DEFAULT_USE_META_REPLICAS);
+ if (useMetaReplicas) {
+ this.metaReplicaMode = CatalogReplicaMode.HedgedRead;
+ }
+ }
+ break;
+ default:
+ // Doing nothing
Review comment:
How would I get here? Needs a log?
----------------------------------------------------------------
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:
[email protected]
