keith-turner commented on code in PR #4685:
URL: https://github.com/apache/accumulo/pull/4685#discussion_r1649787694
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -59,12 +59,18 @@ public class Utils {
public static void checkTableDoesNotExist(ServerContext context, String
tableName,
TableId tableId, TableOperation operation) throws
AcceptableThriftTableOperationException {
- TableId id = context.getTableNameToIdMap().get(tableName);
-
- if (id != null && !id.equals(tableId)) {
- throw new AcceptableThriftTableOperationException(null, tableName,
operation,
- TableOperationExceptionType.EXISTS, null);
+ try {
+ if (context.getZooReader()
+ .exists(context.getZooKeeperRoot() + Constants.ZTABLES + "/" +
tableId.canonical())) {
Review Comment:
I think this function needs a better name, should probably be named
`checkTableNameDoesNotExists`. Looking at how its used I think it needs to
ensure that no table exists with the given name or if it does exists that it
has the specified table id. The changes to this function do not check all
tables ids in zookeeper to ensure they do not have the specified name. The old
code was doing this check, which requires reading all of the table ids in
zookeeper. The function probably needs some javadoc too in addition to a
better name.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -59,12 +59,18 @@ public class Utils {
public static void checkTableDoesNotExist(ServerContext context, String
tableName,
TableId tableId, TableOperation operation) throws
AcceptableThriftTableOperationException {
- TableId id = context.getTableNameToIdMap().get(tableName);
-
- if (id != null && !id.equals(tableId)) {
- throw new AcceptableThriftTableOperationException(null, tableName,
operation,
- TableOperationExceptionType.EXISTS, null);
+ try {
+ if (context.getZooReader()
+ .exists(context.getZooKeeperRoot() + Constants.ZTABLES + "/" +
tableId.canonical())) {
Review Comment:
I think this function needs a better name, should probably be named
`checkTableNameDoesNotExists`. Looking at how its used I think it needs to
ensure that no table exists with the given name or if it does exists that it
has the specified table id. The changes to this function do not check all
tables ids in zookeeper to ensure they do not have the specified name. The old
code was doing this check, which requires reading all of the table ids in
zookeeper and looking at the name under the id. The function probably needs
some javadoc too in addition to a better name.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -59,12 +59,18 @@ public class Utils {
public static void checkTableDoesNotExist(ServerContext context, String
tableName,
TableId tableId, TableOperation operation) throws
AcceptableThriftTableOperationException {
- TableId id = context.getTableNameToIdMap().get(tableName);
-
- if (id != null && !id.equals(tableId)) {
- throw new AcceptableThriftTableOperationException(null, tableName,
operation,
- TableOperationExceptionType.EXISTS, null);
+ try {
+ if (context.getZooReader()
+ .exists(context.getZooKeeperRoot() + Constants.ZTABLES + "/" +
tableId.canonical())) {
Review Comment:
I think this function needs a better name, should probably be named
`checkTableNameDoesNotExists`. Looking at how its used I think it needs to
ensure that no table exists with the given name or if it does exists that it
has the specified table id. These changes check that the table id does not
exist in ZK, but not the that the table name does not exists in ZK. The old
code was doing this check, which requires reading all of the table ids in
zookeeper and looking at the name under the id. The function probably needs
some javadoc too in addition to a better 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]
