ctubbsii commented on code in PR #5334:
URL: https://github.com/apache/accumulo/pull/5334#discussion_r1956449211
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java:
##########
@@ -50,26 +50,23 @@ public Repo<Manager> call(long tid, Manager environment) {
// 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,
+ if (cloneInfo.isKeepOffline()) {
+
environment.getTableManager().transitionTableState(cloneInfo.getTableId(),
TableState.OFFLINE,
expectedCurrStates);
} else {
- environment.getTableManager().transitionTableState(cloneInfo.tableId,
TableState.ONLINE,
+
environment.getTableManager().transitionTableState(cloneInfo.getTableId(),
TableState.ONLINE,
expectedCurrStates);
}
+ Utils.unreserveNamespace(environment, cloneInfo.getNamespaceId(), tid,
false);
+ Utils.unreserveTable(environment, cloneInfo.getSrcTableId(), tid, false);
+ Utils.unreserveTable(environment, cloneInfo.getTableId(), tid, true);
Review Comment:
It probably doesn't matter, but I'd suggest releasing this write lock first.
It's the most restrictive, and the most recently reserved, so it'd be good to
roll back first.
##########
test/src/main/java/org/apache/accumulo/test/functional/CloneTestIT.java:
##########
@@ -350,4 +353,64 @@ public void testCloneMetadataTable() {
"mc1", CloneConfiguration.empty()));
}
}
+
+ @Test
+ public void testCloneIntoDiffNamespace() throws Exception {
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ String tableName = getUniqueNames(1)[0];
+ client.tableOperations().create(tableName);
+ client.namespaceOperations().create("new");
+ client.tableOperations().clone(tableName, "new." + tableName,
CloneConfiguration.empty());
+ }
+ }
+
+ @Test
+ public void testCloneIntoDiffNamespaceDoesntExist() throws Exception {
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ String tableName = getUniqueNames(1)[0];
+ client.tableOperations().create(tableName);
+ assertThrows(AccumuloException.class, () ->
client.tableOperations().clone(tableName,
+ "missing." + tableName, CloneConfiguration.empty()));
+ }
+ }
+
+ @Test
+ public void testCloneIntoAccumuloNamespace() throws Exception {
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ String tableName = getUniqueNames(1)[0];
+ client.tableOperations().create(tableName);
+ assertThrows(AccumuloException.class, () ->
client.tableOperations().clone(tableName,
+ "accumulo." + tableName, CloneConfiguration.empty()));
+ }
+ }
+
+ @Test
+ public void testCloneNamespacePermissions() throws Exception {
+ final String tableName = getUniqueNames(1)[0];
+ final String newUserName = "NEW_USER";
+ final String srcNs = "src";
+ final String srcTableName = srcNs + "." + tableName;
+ final String destNs = "dst";
+ final String dstTableName = destNs + "." + tableName;
+
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ client.namespaceOperations().create(srcNs);
+ client.tableOperations().create(srcTableName);
+ client.namespaceOperations().create(destNs);
+ client.securityOperations().createLocalUser(newUserName, new
PasswordToken(newUserName));
+ // User needs WRITE or ALTER_TABLE on the src table to flush it as part
of the clone operation
+ client.securityOperations().grantTablePermission(newUserName,
srcTableName,
+ TablePermission.ALTER_TABLE);
+ client.securityOperations().grantTablePermission(newUserName,
srcTableName,
+ TablePermission.READ);
Review Comment:
This is the expected success case. However, prior to this fix, I believe if
this was set on the destination namespace, but *NOT* on the source table, then
the operation would still succeed. The test should check that case, to make
sure that it succeeds before this fix (to verify the bug), and fails after this
fix.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneTable.java:
##########
@@ -34,23 +34,18 @@ public class CloneTable extends ManagerRepo {
private static final long serialVersionUID = 1L;
private final CloneInfo cloneInfo;
- public CloneTable(String user, NamespaceId namespaceId, TableId srcTableId,
String tableName,
- Map<String,String> propertiesToSet, Set<String> propertiesToExclude,
boolean keepOffline) {
- cloneInfo = new CloneInfo();
- cloneInfo.user = user;
- cloneInfo.srcTableId = srcTableId;
- cloneInfo.tableName = tableName;
- cloneInfo.propertiesToExclude = propertiesToExclude;
- cloneInfo.propertiesToSet = propertiesToSet;
- cloneInfo.srcNamespaceId = namespaceId;
- cloneInfo.keepOffline = keepOffline;
+ public CloneTable(String user, NamespaceId srcNamespaceId, TableId
srcTableId,
+ NamespaceId namespaceId, String tableName, Map<String,String>
propertiesToSet,
+ Set<String> propertiesToExclude, boolean keepOffline) {
+ cloneInfo = new CloneInfo(srcNamespaceId, srcTableId, namespaceId,
tableName, propertiesToSet,
+ propertiesToExclude, keepOffline, user);
}
@Override
public long isReady(long tid, Manager environment) throws Exception {
- long val = Utils.reserveNamespace(environment, cloneInfo.srcNamespaceId,
tid, false, true,
+ long val = Utils.reserveNamespace(environment, cloneInfo.getNamespaceId(),
tid, false, true,
TableOperation.CLONE);
- val += Utils.reserveTable(environment, cloneInfo.srcTableId, tid, false,
true,
+ val += Utils.reserveTable(environment, cloneInfo.getSrcTableId(), tid,
false, true,
Review Comment:
I think this is right, but I don't know fate well enough. I'd like to hear
from @keith-turner about whether this is sufficient for the clone operation:
1. reserve destination namespace (READ lock)
2. reserve source table (READ lock)
I don't think we need to reserve the source namespace as well (I think
reserving the source table is sufficient), but I'm not 100% certain about that.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneZookeeper.java:
##########
@@ -37,20 +35,12 @@ class CloneZookeeper extends ManagerRepo {
public CloneZookeeper(CloneInfo cloneInfo, ClientContext context)
throws NamespaceNotFoundException {
this.cloneInfo = cloneInfo;
- this.cloneInfo.namespaceId = Namespaces.getNamespaceId(context,
Review Comment:
This is going to change the serialization, and could cause problems with a
2.1.4 upgrade if there are any ongoing clone operations. That should be noted
in the release notes for 2.1.4.
##########
test/src/main/java/org/apache/accumulo/test/functional/CloneTestIT.java:
##########
@@ -350,4 +353,64 @@ public void testCloneMetadataTable() {
"mc1", CloneConfiguration.empty()));
}
}
+
+ @Test
+ public void testCloneIntoDiffNamespace() throws Exception {
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ String tableName = getUniqueNames(1)[0];
+ client.tableOperations().create(tableName);
+ client.namespaceOperations().create("new");
+ client.tableOperations().clone(tableName, "new." + tableName,
CloneConfiguration.empty());
Review Comment:
This tests cloning from the default namespace to a user-created namespace. I
think this test can be expanded by adding:
1. test additional clone succeeds cloning into a second user-defined
namespace, i.e. from new.tableName to new2.tableName
2. insert a few entries and ensure successfully cloned table(s) contains
same data
3. test clone fails when target namespace contains a table of the same name
already
4. test clone fails when cloning to self (same namespace, same table name)
--
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]
