keith-turner commented on code in PR #3294:
URL: https://github.com/apache/accumulo/pull/3294#discussion_r1165683318
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java:
##########
@@ -1014,39 +1014,15 @@ default TimeType getTimeType(String tableName) throws
TableNotFoundException {
}
/**
- * Initiates setting a table to an on-demand state, but does not wait for
action to complete
+ * Sets the hosting goal for a range of Tablets in the specified table.
*
- * @param tableName the table to set to an on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @throws AccumuloSecurityException when the user does not have the proper
permissions
- * @since 3.1.0
- */
- void onDemand(String tableName)
- throws AccumuloSecurityException, AccumuloException,
TableNotFoundException;
-
- /**
- * Initiates setting a table to an on-demand state, optionally waits for
action to complete
- *
- * @param tableName the table to set to an on-demand state
- * @param wait if true, then will not return until table state is set to an
on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @throws AccumuloSecurityException when the user does not have the proper
permissions
- * @since 3.1.0
- */
- void onDemand(String tableName, boolean wait)
- throws AccumuloSecurityException, AccumuloException,
TableNotFoundException;
-
- /**
- * Check if a table is on-demand through its current goal state only. Could
run into issues if the
- * current state of the table is in between states. If you require a
specific state, call
- * <code>onDemand(tableName, true)</code>, this will wait until the table
reaches the desired
- * state before proceeding.
- *
- * @param tableName the table to check if in an on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @return true if table's goal state is online
- * @since 3.1.0
+ * @param tableName table name
+ * @param range tablet range
+ * @param goal hosting goal (one of {@literal ALWAYS, DEFAULT, NEVER})
+ * @since ELASTICITY_TODO
*/
- boolean isOnDemand(String tableName) throws AccumuloException,
TableNotFoundException;
-
+ default void setTabletHostingGoal(String tableName, Range range, String goal)
Review Comment:
The goal parameter should probably be an enum. Could use the
TabletHostingGoal minus the static methods, maybe have a TabletHostingGoalImpl
class that has the static methods.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java:
##########
@@ -2175,4 +2150,25 @@ private void validatePropertiesToSet(Map<String,String>
opts, Map<String,String>
opts.put(k, v);
});
}
+
+ @Override
+ public void setTabletHostingGoal(String tableName, Range range, String goal)
+ throws AccumuloSecurityException, AccumuloException,
TableNotFoundException {
+ EXISTING_TABLE_NAME.validate(tableName);
+ NOT_BUILTIN_TABLE.validate(tableName);
+ checkArgument(range != null, "range is null");
+ checkArgument(goal != null, "goal is null");
+
+ TableId tableId = context.getTableId(tableName);
+ TabletHostingGoal g = TabletHostingGoal.valueOf(goal.toUpperCase());
+
+ List<TKeyExtent> extents =
+ TabletLocatorImpl.findExtentsForRange(context, tableId, range,
Set.of());
+
+ log.debug("Setting tablet hosting goal to {} for extents: {}", goal,
extents);
+ ThriftClientTypes.TABLET_MGMT.executeVoid(context,
+ client -> client.setTabletHostingGoal(TraceUtil.traceInfo(),
context.rpcCreds(),
+ tableId.canonical(), extents, g.toThrift()));
Review Comment:
Instead of resolving the range into extents on the client side, it would
probably be better to pass the range to the manager and let it do the
resolution. Could also make this a FATE operations so that the entire range is
processed even in the case of failures.
Making it a FATE operation and passing the range to the manager could be
follow on work/PR.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java:
##########
@@ -1014,39 +1014,15 @@ default TimeType getTimeType(String tableName) throws
TableNotFoundException {
}
/**
- * Initiates setting a table to an on-demand state, but does not wait for
action to complete
+ * Sets the hosting goal for a range of Tablets in the specified table.
*
- * @param tableName the table to set to an on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @throws AccumuloSecurityException when the user does not have the proper
permissions
- * @since 3.1.0
- */
- void onDemand(String tableName)
- throws AccumuloSecurityException, AccumuloException,
TableNotFoundException;
-
- /**
- * Initiates setting a table to an on-demand state, optionally waits for
action to complete
- *
- * @param tableName the table to set to an on-demand state
- * @param wait if true, then will not return until table state is set to an
on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @throws AccumuloSecurityException when the user does not have the proper
permissions
- * @since 3.1.0
- */
- void onDemand(String tableName, boolean wait)
- throws AccumuloSecurityException, AccumuloException,
TableNotFoundException;
-
- /**
- * Check if a table is on-demand through its current goal state only. Could
run into issues if the
- * current state of the table is in between states. If you require a
specific state, call
- * <code>onDemand(tableName, true)</code>, this will wait until the table
reaches the desired
- * state before proceeding.
- *
- * @param tableName the table to check if in an on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @return true if table's goal state is online
- * @since 3.1.0
+ * @param tableName table name
+ * @param range tablet range
+ * @param goal hosting goal (one of {@literal ALWAYS, DEFAULT, NEVER})
+ * @since ELASTICITY_TODO
*/
- boolean isOnDemand(String tableName) throws AccumuloException,
TableNotFoundException;
-
+ default void setTabletHostingGoal(String tableName, Range range, String goal)
+ throws AccumuloSecurityException, AccumuloException,
TableNotFoundException {
+ throw new UnsupportedOperationException();
+ }
Review Comment:
Users will need a way to inspect what hosting goals are currently set.
Could have something like the following.
```java
static class HostingGoalForTablet {
final TabletId tablet;
final TabletHostingGoal hostingGoal;
}
/**
* Retrieves a stream of tablet hosting goals for tablets that fall within
the range.
*/
Stream<HostingGoalForTablet> getTabletHostingGoals(String tableName, Range
range)
```
Or could have something that collapses consecutive tablets that have the
same goal into the same range.
```java
static class TabletHostingGoalRange {
final Text startRow;
final Text endRow;
final TabletHostingGoal hostingGoal;
}
/**
* Retrieves a stream of tablet hosting goals for a tablet. Consecutive
tablets that have same goal are collapsed to form a single
TabletHostingGoalRange.
*/
Stream<TabletHostingGoalRange> getTabletHostingGoals(String tableName, Range
range)
```
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocatorImpl.java:
##########
@@ -542,70 +549,99 @@ public TabletLocation locateTablet(ClientContext context,
Text row, boolean skip
}
@Override
- public long onDemandTabletsOnlined() {
- return onDemandTabletsOnlinedCount.get();
+ public long getTabletHostingRequestCount() {
+ return tabletHostingRequestCount.get();
+ }
+
+ @VisibleForTesting
+ public void resetTabletHostingRequestCount() {
+ tabletHostingRequestCount.set(0);
}
- private void bringOnDemandTabletsOnline(ClientContext context, Range range)
+ @VisibleForTesting
+ public void enableTabletHostingRequests(boolean enabled) {
+ HOSTING_ENABLED.set(enabled);
+ }
+
+ private void requestTabletHosting(ClientContext context, Range range)
throws AccumuloException, AccumuloSecurityException {
- // Confirm that table is in an on-demand state. Don't throw an exception
+ if (!HOSTING_ENABLED.get()) {
+ return;
+ }
+
+ // System tables should always be hosted
+ if (RootTable.ID == tableId || MetadataTable.ID == tableId) {
+ return;
+ }
+
+ // Confirm that table is in an online state. Don't throw an exception
// if the table is not found, calling code will already handle it.
try {
String tableName = context.getTableName(tableId);
- if (!context.tableOperations().isOnDemand(tableName)) {
- log.trace("bringOnDemandTabletsOnline: table {} is not in ondemand
state", tableId);
+ if (!context.tableOperations().isOnline(tableName)) {
+ log.trace("requestTabletHosting: table {} is not online", tableId);
return;
}
} catch (TableNotFoundException e) {
- log.trace("bringOnDemandTabletsOnline: table not found: {}", tableId);
+ log.trace("requestTabletHosting: table not found: {}", tableId);
return;
}
- final Text scanRangeStart = range.getStartKey().getRow();
- final Text scanRangeEnd = range.getEndKey().getRow();
- // Turn the scan range into a KeyExtent and bring online all ondemand
tablets
+ List<TKeyExtent> extentsToBringOnline =
Review Comment:
In #3292 I was trying to minimize the amount of reads to the metadata
table. Like maybe we just read this information to populate the cache and now
are reading the metadata table again. I think it would be best to do this its
own PR so I don't think any changes are needed in this PR. I am not completely
happy with what I did in #3292 so it would be nice to focus on that one issue
rather than do it as part of larger changes.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocatorImpl.java:
##########
@@ -542,70 +549,99 @@ public TabletLocation locateTablet(ClientContext context,
Text row, boolean skip
}
@Override
- public long onDemandTabletsOnlined() {
- return onDemandTabletsOnlinedCount.get();
+ public long getTabletHostingRequestCount() {
+ return tabletHostingRequestCount.get();
+ }
+
+ @VisibleForTesting
+ public void resetTabletHostingRequestCount() {
+ tabletHostingRequestCount.set(0);
}
- private void bringOnDemandTabletsOnline(ClientContext context, Range range)
+ @VisibleForTesting
+ public void enableTabletHostingRequests(boolean enabled) {
+ HOSTING_ENABLED.set(enabled);
+ }
+
+ private void requestTabletHosting(ClientContext context, Range range)
throws AccumuloException, AccumuloSecurityException {
- // Confirm that table is in an on-demand state. Don't throw an exception
+ if (!HOSTING_ENABLED.get()) {
+ return;
+ }
+
+ // System tables should always be hosted
+ if (RootTable.ID == tableId || MetadataTable.ID == tableId) {
+ return;
+ }
+
+ // Confirm that table is in an online state. Don't throw an exception
// if the table is not found, calling code will already handle it.
try {
String tableName = context.getTableName(tableId);
- if (!context.tableOperations().isOnDemand(tableName)) {
- log.trace("bringOnDemandTabletsOnline: table {} is not in ondemand
state", tableId);
+ if (!context.tableOperations().isOnline(tableName)) {
+ log.trace("requestTabletHosting: table {} is not online", tableId);
return;
}
} catch (TableNotFoundException e) {
- log.trace("bringOnDemandTabletsOnline: table not found: {}", tableId);
+ log.trace("requestTabletHosting: table not found: {}", tableId);
Review Comment:
This should probably throw an exception.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java:
##########
@@ -332,10 +332,16 @@ public static class ExternalCompactionColumnFamily {
public static final Text NAME = new Text(STR_NAME);
}
- public static class OnDemandAssignmentStateColumnFamily {
- public static final String STR_NAME = "ondemand";
+ public static class HostingGoalColumnFamily {
+ public static final String STR_NAME = "hosting_goal";
public static final Text NAME = new Text(STR_NAME);
+ public static final String ALWAYS = "always"; // tablet should always be
hosted
+ public static final String DEFAULT = "default"; // tablet should be
unassigned
+ public static final String ONDEMAND = "ondemand"; // ondemand request
has been made to assign
+ // tablet
+ public static final String NEVER = "never"; // tablet should never be
hosted
Review Comment:
```suggestion
public static final String ALWAYS = TabletHostingGoal.ALWAYS.name();
// tablet should always be hosted
public static final String DEFAULT = TabletHostingGoal.DEFAULT.name();
// tablet should be unassigned
public static final String ONDEMAND =
TabletHostingGoal.ONDEMAND.name(); // ondemand request has been made to assign
// tablet
public static final String NEVER = TabletHostingGoal.NEVER.name(); //
tablet should never be hosted
```
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java:
##########
@@ -1014,39 +1014,15 @@ default TimeType getTimeType(String tableName) throws
TableNotFoundException {
}
/**
- * Initiates setting a table to an on-demand state, but does not wait for
action to complete
+ * Sets the hosting goal for a range of Tablets in the specified table.
*
- * @param tableName the table to set to an on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @throws AccumuloSecurityException when the user does not have the proper
permissions
- * @since 3.1.0
- */
- void onDemand(String tableName)
- throws AccumuloSecurityException, AccumuloException,
TableNotFoundException;
-
- /**
- * Initiates setting a table to an on-demand state, optionally waits for
action to complete
- *
- * @param tableName the table to set to an on-demand state
- * @param wait if true, then will not return until table state is set to an
on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @throws AccumuloSecurityException when the user does not have the proper
permissions
- * @since 3.1.0
- */
- void onDemand(String tableName, boolean wait)
- throws AccumuloSecurityException, AccumuloException,
TableNotFoundException;
-
- /**
- * Check if a table is on-demand through its current goal state only. Could
run into issues if the
- * current state of the table is in between states. If you require a
specific state, call
- * <code>onDemand(tableName, true)</code>, this will wait until the table
reaches the desired
- * state before proceeding.
- *
- * @param tableName the table to check if in an on-demand state
- * @throws AccumuloException when there is a general accumulo error
- * @return true if table's goal state is online
- * @since 3.1.0
+ * @param tableName table name
+ * @param range tablet range
+ * @param goal hosting goal (one of {@literal ALWAYS, DEFAULT, NEVER})
Review Comment:
Is DEFAULT like ondemand? I do not have a sense of what it means from the
name. ALWAYS and NEVER are very clear to me.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
