You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/04/13 15:53:23 UTC
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3294: Modify all user tables to behave in OnDemand manner
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org