ctubbsii commented on code in PR #5323:
URL: https://github.com/apache/accumulo/pull/5323#discussion_r1951588874
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneTable.java:
##########
@@ -42,7 +42,7 @@ public CloneTable(String user, NamespaceId namespaceId,
TableId srcTableId, Stri
cloneInfo.tableName = tableName;
cloneInfo.propertiesToExclude = propertiesToExclude;
cloneInfo.propertiesToSet = propertiesToSet;
- cloneInfo.srcNamespaceId = namespaceId;
+ cloneInfo.srcNamespaceId = namespaceId; // TODO this seems like the dest
table id
Review Comment:
#5324
##########
test/src/main/java/org/apache/accumulo/test/TableOperationsIT.java:
##########
@@ -216,6 +218,81 @@ public void createTableWithBadProperties()
() -> tableOps.setProperty(t0, Property.TABLE_BLOOM_ENABLED.getKey(),
"foo"));
}
+ @Test
+ public void createTablesWithSameNameInDifferentNamespace() throws Exception {
+ TableOperations tableOps = accumuloClient.tableOperations();
+ String[] names = getUniqueNames(2);
+
+ accumuloClient.namespaceOperations().create("test1");
+ accumuloClient.namespaceOperations().create("test2");
+
+ var tables = Set.of("test1.table1", "test1.table2", "test1.root",
"test2.table1",
+ "test2.table2", "test2.metadata", "table1", "table2", "metadata");
+
+ for (String table : tables) {
+ tableOps.create(table);
+ }
+
+ assertTrue(accumuloClient.tableOperations().list().containsAll(tables));
+
+ accumuloClient.namespaceOperations().create("test3");
+
+ Set<String> clones = new HashSet<>();
+ for (String table : tables) {
+ if (table.startsWith("test1.")) {
+ var clone = table.replace("test1.", "test3.");
+ tableOps.clone(table, clone, CloneConfiguration.empty());
+ clones.add(clone);
+ }
+ }
+
+ assertTrue(accumuloClient.tableOperations().list().containsAll(tables));
+ assertTrue(accumuloClient.tableOperations().list().containsAll(clones));
+
+ // Rename a table to a tablename that exists in another namespace
+ tableOps.rename("test1.table1", "test1.metadata");
+
+ tables = new HashSet<>(tables);
+ tables.remove("test1.table1");
+ tables.add("test1.metadata");
+
+ assertTrue(accumuloClient.tableOperations().list().containsAll(tables));
+ assertTrue(accumuloClient.tableOperations().list().containsAll(clones));
+
assertFalse(accumuloClient.tableOperations().list().contains("test1.table"));
+
+ // Read and write data to tables w/ the same name in different namespaces
to ensure no wires are
+ // crossed.
+ for (var table : Sets.union(tables, clones)) {
+ try (var writer = accumuloClient.createBatchWriter(table)) {
+ Mutation m = new Mutation("self");
+ m.put("table", "name", table);
+ writer.addMutation(m);
+ }
+ }
+
+ for (var table : Sets.union(tables, clones)) {
+ try (var scanner = accumuloClient.createScanner(table)) {
+ var seenValues =
+ scanner.stream().map(e ->
e.getValue().toString()).collect(Collectors.toSet());
+ assertEquals(Set.of(table), seenValues);
+ }
+ }
+
+ for (var table : Sets.union(tables, clones)) {
+ assertThrows(TableExistsException.class,
+ () -> accumuloClient.tableOperations().create(table));
+ }
+
+ for (var srcTable : List.of("metadata", "test1.table2")) {
+ for (var destTable : Sets.union(tables, clones)) {
+ assertThrows(TableExistsException.class, () ->
accumuloClient.tableOperations()
+ .clone(srcTable, destTable, CloneConfiguration.empty()));
+ }
+ }
+
+ // TODO test export/import w/ diff table names
Review Comment:
Do you have a follow-up issue already created for this?
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -68,47 +62,29 @@ public class Utils {
* Checks that a table name is only used by the specified table id or not
used at all.
*/
public static void checkTableNameDoesNotExist(ServerContext context, String
tableName,
- TableId tableId, TableOperation operation) throws
AcceptableThriftTableOperationException {
+ NamespaceId destNamespaceId, TableId tableId, TableOperation operation)
+ throws AcceptableThriftTableOperationException {
+
+ var newTableName = TableNameUtil.qualify(tableName).getSecond();
- final Map<NamespaceId,String> namespaces = new HashMap<>();
- final boolean namespaceInTableName = tableName.contains(".");
try {
for (String tid : context.getZooReader()
.getChildren(context.getZooKeeperRoot() + Constants.ZTABLES)) {
final String zTablePath = context.getZooKeeperRoot() +
Constants.ZTABLES + "/" + tid;
try {
final byte[] tname = context.getZooReader().getData(zTablePath +
Constants.ZTABLE_NAME);
- Preconditions.checkState(tname != null, "Malformed table entry in
ZooKeeper at %s",
- zTablePath);
- String namespaceName = Namespace.DEFAULT.name();
- if (namespaceInTableName) {
+ if (newTableName.equals(new String(tname, UTF_8))) {
+ // only make RPCs to get the namespace when the table names are
equal
final byte[] nId =
context.getZooReader().getData(zTablePath +
Constants.ZTABLE_NAMESPACE);
- if (nId != null) {
- final NamespaceId namespaceId = NamespaceId.of(new String(nId,
UTF_8));
- if (!namespaceId.equals(Namespace.DEFAULT.id())) {
- namespaceName = namespaces.get(namespaceId);
- if (namespaceName == null) {
- try {
- namespaceName = Namespaces.getNamespaceName(context,
namespaceId);
- namespaces.put(namespaceId, namespaceName);
- } catch (NamespaceNotFoundException e) {
- throw new AcceptableThriftTableOperationException(null,
tableName,
- TableOperation.CREATE,
TableOperationExceptionType.OTHER,
- "Table (" + tableId.canonical() + ") contains
reference to namespace ("
- + namespaceId + ") that doesn't exist");
- }
- }
- }
+ if (destNamespaceId.canonical().equals(new String(nId, UTF_8))
+ && !tableId.equals(TableId.of(tid))) {
Review Comment:
I'm wondering if we want to short-circuit the for loop if the tableId is the
same, or continue the loop to verify no others have the same name.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -68,47 +62,29 @@ public class Utils {
* Checks that a table name is only used by the specified table id or not
used at all.
*/
public static void checkTableNameDoesNotExist(ServerContext context, String
tableName,
- TableId tableId, TableOperation operation) throws
AcceptableThriftTableOperationException {
+ NamespaceId destNamespaceId, TableId tableId, TableOperation operation)
+ throws AcceptableThriftTableOperationException {
+
+ var newTableName = TableNameUtil.qualify(tableName).getSecond();
- final Map<NamespaceId,String> namespaces = new HashMap<>();
- final boolean namespaceInTableName = tableName.contains(".");
try {
for (String tid : context.getZooReader()
.getChildren(context.getZooKeeperRoot() + Constants.ZTABLES)) {
final String zTablePath = context.getZooKeeperRoot() +
Constants.ZTABLES + "/" + tid;
try {
final byte[] tname = context.getZooReader().getData(zTablePath +
Constants.ZTABLE_NAME);
- Preconditions.checkState(tname != null, "Malformed table entry in
ZooKeeper at %s",
- zTablePath);
- String namespaceName = Namespace.DEFAULT.name();
- if (namespaceInTableName) {
+ if (newTableName.equals(new String(tname, UTF_8))) {
+ // only make RPCs to get the namespace when the table names are
equal
final byte[] nId =
context.getZooReader().getData(zTablePath +
Constants.ZTABLE_NAMESPACE);
- if (nId != null) {
- final NamespaceId namespaceId = NamespaceId.of(new String(nId,
UTF_8));
- if (!namespaceId.equals(Namespace.DEFAULT.id())) {
- namespaceName = namespaces.get(namespaceId);
- if (namespaceName == null) {
- try {
- namespaceName = Namespaces.getNamespaceName(context,
namespaceId);
- namespaces.put(namespaceId, namespaceName);
- } catch (NamespaceNotFoundException e) {
- throw new AcceptableThriftTableOperationException(null,
tableName,
- TableOperation.CREATE,
TableOperationExceptionType.OTHER,
- "Table (" + tableId.canonical() + ") contains
reference to namespace ("
- + namespaceId + ") that doesn't exist");
- }
- }
- }
+ if (destNamespaceId.canonical().equals(new String(nId, UTF_8))
+ && !tableId.equals(TableId.of(tid))) {
Review Comment:
Could avoid an object creation here, and to make the comparison consistent
with the other clause in this expression.
```suggestion
&& !tableId.canonical().equals(tid)) {
```
--
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]
