[GitHub] [accumulo] keith-turner commented on a diff in pull request #3220: Add ondemand table state

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #3220:
URL: https://github.com/apache/accumulo/pull/3220#discussion_r1142390616


##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -418,6 +417,32 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
             goalMessage);
         break;
       }
+      case TABLE_ONDEMAND: {
+        TableOperation tableOp = TableOperation.ONDEMAND;
+        validateArgumentCount(arguments, tableOp, 1);
+        final var tableId = validateTableIdArgument(arguments.get(0), tableOp,
+            NOT_ROOT_TABLE_ID.and(NOT_METADATA_TABLE_ID));

Review Comment:
   Feel like this check should be consistent with TABLE_OFFLINE which only seems to check if its root.  Seems like TABLE_OFFLINE should also check if its metadata table.



##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java:
##########
@@ -1479,36 +1463,90 @@ public void online(String tableName)
     online(tableName, false);
   }
 
-  @Override
-  public void online(String tableName, boolean wait)
+  private void changeTableState(String tableName, boolean wait, TableState newState)
       throws AccumuloSecurityException, AccumuloException, TableNotFoundException {
     EXISTING_TABLE_NAME.validate(tableName);
 
     TableId tableId = context.getTableId(tableName);
-    /**
-     * ACCUMULO-4574 if table is already online return without executing fate operation.
-     */
-    if (isOnline(tableName)) {
-      if (wait) {
-        waitForTableStateTransition(tableId, TableState.ONLINE);
-      }
-      return;
+
+    FateOperation op = null;
+    switch (newState) {
+      case OFFLINE:
+        op = FateOperation.TABLE_OFFLINE;
+        break;
+      case ONDEMAND:
+        op = FateOperation.TABLE_ONDEMAND;
+        if (tableName.equals(MetadataTable.NAME) || tableName.equals(RootTable.NAME)) {

Review Comment:
   OFFLINE has similar restrictions but does not do these checks client side. For consistency, I think neither or both OFFLINE and ONDEMAND should do the checks client side.



-- 
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