This is an automated email from the ASF dual-hosted git repository.
domgarguilo pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new cefa3a405d Stricter checks before transitioning table state (#4165)
cefa3a405d is described below
commit cefa3a405dd2355157743629fd6350334b421eb3
Author: Kevin Rathbun <[email protected]>
AuthorDate: Wed Jan 17 12:59:26 2024 -0500
Stricter checks before transitioning table state (#4165)
Prior to these changes, users were able to manually transition a table from
the NEW state to another state. More specifically, calls to
TableOperations.online() and TableOperations.offline() when the table was in
the NEW state were acceptable. Users should not be able to transition a table
from the NEW state as this should only be done by the system upon successfully
creating/cloning/importing the table.
Changes:
- Added new param expectedCurrStates to TableManager.transitionTableState()
- Users can now only manually change a table from
[ONLINE/OFFLINE]->[ONLINE/OFFLINE] (FateServiceHandler)
- Added field expectedCurrStates to ChangeTableState
- Table clones, creations, and imports now explicitly expect the current
state before completion to be NEW (FinishCloneTable, FinishCreateTable,
FinishImportTable)
- Table deletions now explicitly expect the current state before deletion
to be only ONLINE or OFFLINE (DeleteTable)
---
.../accumulo/server/tables/TableManager.java | 22 ++++++++++++----------
.../accumulo/manager/FateServiceHandler.java | 16 ++++++++++++----
.../manager/tableOps/ChangeTableState.java | 9 +++++++--
.../manager/tableOps/clone/FinishCloneTable.java | 9 +++++++--
.../manager/tableOps/create/FinishCreateTable.java | 6 ++++--
.../manager/tableOps/delete/DeleteTable.java | 6 +++++-
.../tableOps/tableImport/FinishImportTable.java | 5 ++++-
7 files changed, 51 insertions(+), 22 deletions(-)
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java
b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java
index 215129cc79..29df98fc25 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java
@@ -21,6 +21,7 @@ package org.apache.accumulo.server.tables;
import static java.nio.charset.StandardCharsets.UTF_8;
import java.util.Collections;
+import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
@@ -134,19 +135,20 @@ public class TableManager {
return tableStateCache.get(tableId);
}
- public synchronized void transitionTableState(final TableId tableId, final
TableState newState) {
+ public synchronized void transitionTableState(final TableId tableId, final
TableState newState,
+ final EnumSet<TableState> expectedCurrStates) {
Preconditions.checkArgument(newState != TableState.UNKNOWN);
String statePath = zkRoot + Constants.ZTABLES + "/" + tableId +
Constants.ZTABLE_STATE;
try {
- zoo.mutateOrCreate(statePath, newState.name().getBytes(UTF_8), oldData
-> {
- TableState oldState = TableState.UNKNOWN;
- if (oldData != null) {
- oldState = TableState.valueOf(new String(oldData, UTF_8));
+ zoo.mutateOrCreate(statePath, newState.name().getBytes(UTF_8), currData
-> {
+ TableState currState = TableState.UNKNOWN;
+ if (currData != null) {
+ currState = TableState.valueOf(new String(currData, UTF_8));
}
// this check makes the transition operation idempotent
- if (oldState == newState) {
+ if (currState == newState) {
return null; // already at desired state, so nothing to do
}
@@ -154,7 +156,7 @@ public class TableManager {
// +--------+
// v |
// NEW -> (ONLINE|OFFLINE)+--- DELETING
- switch (oldState) {
+ switch (currState) {
case NEW:
transition = (newState == TableState.OFFLINE || newState ==
TableState.ONLINE);
break;
@@ -168,10 +170,10 @@ public class TableManager {
transition = false;
break;
}
- if (!transition) {
- throw new IllegalTableTransitionException(oldState, newState);
+ if (!transition || !expectedCurrStates.contains(currState)) {
+ throw new IllegalTableTransitionException(currState, newState);
}
- log.debug("Transitioning state for table {} from {} to {}", tableId,
oldState, newState);
+ log.debug("Transitioning state for table {} from {} to {}", tableId,
currState, newState);
return newState.name().getBytes(UTF_8);
});
} catch (Exception e) {
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
index 6c6d0f5a32..0444099675 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
@@ -34,6 +34,7 @@ import static
org.apache.accumulo.core.util.Validators.sameNamespaceAs;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.Base64;
+import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
@@ -61,6 +62,7 @@ import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.data.NamespaceId;
import org.apache.accumulo.core.data.TableId;
import org.apache.accumulo.core.fate.ReadOnlyTStore.TStatus;
+import org.apache.accumulo.core.manager.state.tables.TableState;
import org.apache.accumulo.core.manager.thrift.FateOperation;
import org.apache.accumulo.core.manager.thrift.FateService;
import org.apache.accumulo.core.manager.thrift.ThriftPropertyException;
@@ -388,9 +390,12 @@ class FateServiceHandler implements FateService.Iface {
}
goalMessage += "Online table " + tableId;
+ final EnumSet<TableState> expectedCurrStates =
+ EnumSet.of(TableState.ONLINE, TableState.OFFLINE);
manager.fate().seedTransaction(op.toString(), opid,
- new TraceRepo<>(new ChangeTableState(namespaceId, tableId,
tableOp)), autoCleanup,
- goalMessage);
+ new TraceRepo<>(
+ new ChangeTableState(namespaceId, tableId, tableOp,
expectedCurrStates)),
+ autoCleanup, goalMessage);
break;
}
case TABLE_OFFLINE: {
@@ -413,9 +418,12 @@ class FateServiceHandler implements FateService.Iface {
}
goalMessage += "Offline table " + tableId;
+ final EnumSet<TableState> expectedCurrStates =
+ EnumSet.of(TableState.ONLINE, TableState.OFFLINE);
manager.fate().seedTransaction(op.toString(), opid,
- new TraceRepo<>(new ChangeTableState(namespaceId, tableId,
tableOp)), autoCleanup,
- goalMessage);
+ new TraceRepo<>(
+ new ChangeTableState(namespaceId, tableId, tableOp,
expectedCurrStates)),
+ autoCleanup, goalMessage);
break;
}
case TABLE_MERGE: {
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java
index 38025c916e..146ea96c84 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java
@@ -18,6 +18,8 @@
*/
package org.apache.accumulo.manager.tableOps;
+import java.util.EnumSet;
+
import org.apache.accumulo.core.clientImpl.thrift.TableOperation;
import org.apache.accumulo.core.data.NamespaceId;
import org.apache.accumulo.core.data.TableId;
@@ -32,11 +34,14 @@ public class ChangeTableState extends ManagerRepo {
private TableId tableId;
private NamespaceId namespaceId;
private TableOperation top;
+ private final EnumSet<TableState> expectedCurrStates;
- public ChangeTableState(NamespaceId namespaceId, TableId tableId,
TableOperation top) {
+ public ChangeTableState(NamespaceId namespaceId, TableId tableId,
TableOperation top,
+ EnumSet<TableState> expectedCurrStates) {
this.tableId = tableId;
this.namespaceId = namespaceId;
this.top = top;
+ this.expectedCurrStates = expectedCurrStates;
if (top != TableOperation.ONLINE && top != TableOperation.OFFLINE) {
throw new IllegalArgumentException(top.toString());
@@ -58,7 +63,7 @@ public class ChangeTableState extends ManagerRepo {
ts = TableState.OFFLINE;
}
- env.getTableManager().transitionTableState(tableId, ts);
+ env.getTableManager().transitionTableState(tableId, ts,
expectedCurrStates);
Utils.unreserveNamespace(env, namespaceId, tid, false);
Utils.unreserveTable(env, tableId, tid, true);
LoggerFactory.getLogger(ChangeTableState.class).debug("Changed table state
{} {}", tableId, ts);
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java
index 47107444fa..9c2b46f2a2 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java
@@ -18,6 +18,8 @@
*/
package org.apache.accumulo.manager.tableOps.clone;
+import java.util.EnumSet;
+
import org.apache.accumulo.core.fate.Repo;
import org.apache.accumulo.core.manager.state.tables.TableState;
import org.apache.accumulo.manager.Manager;
@@ -47,10 +49,13 @@ class FinishCloneTable extends ManagerRepo {
// may never create files.. therefore there is no need to consume namenode
space w/ directories
// that are not used... tablet will create directories as needed
+ final EnumSet<TableState> expectedCurrStates = EnumSet.of(TableState.NEW);
if (cloneInfo.keepOffline) {
- environment.getTableManager().transitionTableState(cloneInfo.tableId,
TableState.OFFLINE);
+ environment.getTableManager().transitionTableState(cloneInfo.tableId,
TableState.OFFLINE,
+ expectedCurrStates);
} else {
- environment.getTableManager().transitionTableState(cloneInfo.tableId,
TableState.ONLINE);
+ environment.getTableManager().transitionTableState(cloneInfo.tableId,
TableState.ONLINE,
+ expectedCurrStates);
}
Utils.unreserveNamespace(environment, cloneInfo.srcNamespaceId, tid,
false);
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java
index cdd6ad7291..519ef36141 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java
@@ -19,6 +19,7 @@
package org.apache.accumulo.manager.tableOps.create;
import java.io.IOException;
+import java.util.EnumSet;
import org.apache.accumulo.core.client.admin.InitialTableState;
import org.apache.accumulo.core.fate.Repo;
@@ -50,13 +51,14 @@ class FinishCreateTable extends ManagerRepo {
@Override
public Repo<Manager> call(long tid, Manager env) throws Exception {
+ final EnumSet<TableState> expectedCurrStates = EnumSet.of(TableState.NEW);
if (tableInfo.getInitialTableState() == InitialTableState.OFFLINE) {
env.getContext().getTableManager().transitionTableState(tableInfo.getTableId(),
- TableState.OFFLINE);
+ TableState.OFFLINE, expectedCurrStates);
} else {
env.getContext().getTableManager().transitionTableState(tableInfo.getTableId(),
- TableState.ONLINE);
+ TableState.ONLINE, expectedCurrStates);
}
Utils.unreserveNamespace(env, tableInfo.getNamespaceId(), tid, false);
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
index c578f64b89..fcd0e7984e 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
@@ -18,6 +18,8 @@
*/
package org.apache.accumulo.manager.tableOps.delete;
+import java.util.EnumSet;
+
import org.apache.accumulo.core.clientImpl.thrift.TableOperation;
import org.apache.accumulo.core.data.NamespaceId;
import org.apache.accumulo.core.data.TableId;
@@ -47,7 +49,9 @@ public class DeleteTable extends ManagerRepo {
@Override
public Repo<Manager> call(long tid, Manager env) {
- env.getTableManager().transitionTableState(tableId, TableState.DELETING);
+ final EnumSet<TableState> expectedCurrStates =
+ EnumSet.of(TableState.ONLINE, TableState.OFFLINE);
+ env.getTableManager().transitionTableState(tableId, TableState.DELETING,
expectedCurrStates);
env.getEventCoordinator().event("deleting table %s ", tableId);
return new CleanUp(tableId, namespaceId);
}
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/FinishImportTable.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/FinishImportTable.java
index e5df722043..125e4639e8 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/FinishImportTable.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/FinishImportTable.java
@@ -20,6 +20,8 @@ package org.apache.accumulo.manager.tableOps.tableImport;
import static org.apache.accumulo.core.Constants.IMPORT_MAPPINGS_FILE;
+import java.util.EnumSet;
+
import org.apache.accumulo.core.fate.Repo;
import org.apache.accumulo.core.manager.state.tables.TableState;
import org.apache.accumulo.manager.Manager;
@@ -52,8 +54,9 @@ class FinishImportTable extends ManagerRepo {
}
}
+ final EnumSet<TableState> expectedCurrStates = EnumSet.of(TableState.NEW);
final TableState newState = tableInfo.keepOffline ? TableState.OFFLINE :
TableState.ONLINE;
- env.getTableManager().transitionTableState(tableInfo.tableId, newState);
+ env.getTableManager().transitionTableState(tableInfo.tableId, newState,
expectedCurrStates);
Utils.unreserveNamespace(env, tableInfo.namespaceId, tid, false);
Utils.unreserveTable(env, tableInfo.tableId, tid, true);
