Skip to content

Minor TableOperations changes #5510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Minor TableOperations changes #5510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae64cfd
Minor TableOperations changes:
kevinrr888 Apr 25, 2025
c564e58
ensure table is not system table for offline, online, setTabletAvaila…
kevinrr888 Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import static org.apache.accumulo.core.util.LazySingletons.RANDOM;
import static org.apache.accumulo.core.util.Validators.EXISTING_TABLE_NAME;
import static org.apache.accumulo.core.util.Validators.NEW_TABLE_NAME;
import static org.apache.accumulo.core.util.Validators.NOT_BUILTIN_TABLE;
import static org.apache.accumulo.core.util.threads.ThreadPoolNames.SPLIT_START_POOL;
import static org.apache.accumulo.core.util.threads.ThreadPoolNames.SPLIT_WAIT_POOL;

Expand Down Expand Up @@ -223,8 +222,7 @@ public SortedSet<String> list() {
public boolean exists(String tableName) {
EXISTING_TABLE_NAME.validate(tableName);

if (tableName.equals(SystemTables.METADATA.tableName())
|| tableName.equals(SystemTables.ROOT.tableName())) {
if (SystemTables.containsTableName(tableName)) {
return true;
}

Expand Down Expand Up @@ -1511,15 +1509,13 @@ private void changeTableState(String tableName, boolean wait, TableState newStat
switch (newState) {
case OFFLINE:
op = TFateOperation.TABLE_OFFLINE;
if (tableName.equals(SystemTables.METADATA.tableName())
|| tableName.equals(SystemTables.ROOT.tableName())) {
if (SystemTables.containsTableName(tableName)) {
throw new AccumuloException("Cannot set table to offline state");
}
break;
case ONLINE:
op = TFateOperation.TABLE_ONLINE;
if (tableName.equals(SystemTables.METADATA.tableName())
|| tableName.equals(SystemTables.ROOT.tableName())) {
if (SystemTables.containsTableName(tableName)) {
// Don't submit a Fate operation for this, these tables can only be online.
return;
}
Expand Down Expand Up @@ -2243,7 +2239,10 @@ private void validatePropertiesToSet(Map<String,String> opts, Map<String,String>
public void setTabletAvailability(String tableName, Range range, TabletAvailability availability)
throws AccumuloSecurityException, AccumuloException {
EXISTING_TABLE_NAME.validate(tableName);
NOT_BUILTIN_TABLE.validate(tableName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave this in, but add the check in FateServiceHandler. No reason for the RPC to the Manager when we know it's going to fail.

Copy link
Member Author

@kevinrr888 kevinrr888 Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case we get an IllegalArgumentException for a built in table here instead of a AccumuloException. I agree that there is no need for the RPC beforehand, but this is the pattern taken for all the other methods:

IllegalArgumentException - The table name doesn't make sense. Not expected format
AccumuloException - The table operation cannot be performed on that table

And was probably the original intention with #1953

and I think it should be consistent across all operations. If we want to keep built in table here, we should have it everywhere applicable on client-side

Copy link
Member Author

@kevinrr888 kevinrr888 Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think IllegalArgumentException would also make sense for system tables with the added benefit of avoiding RPC, we just need to decide what is the behavior across all table operations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we have two options, make it consistent everywhere, or keep past behavior. I don't think we need to remove the check to keep past behavior, we can just catch the IllegalArgumentException and raise an AccumuloException instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we would want to catch a IAE then immediately throw AE throughout TableOperations

I would prefer to keep it consistent everywhere. This single method is the only one not consistent with the rest, and I don't think it was intentional.

If we want to go with checking for BUILTIN_TABLE, METADATA_TABLE, or ROOT_TABLE on the client side, maybe that could be it's own PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, maybe a compromise would be to remove NOT_BUILTIN_TABLE, but add:

  if (SystemTables.tableNames().contains(tableName) {
    throw new AccumuloException("Cannot set table availability on a system table");
  }

This is similar to what is happening here

Copy link
Member Author

@kevinrr888 kevinrr888 Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code you linked should have been looking at all system tables (or none and just let it be handled server side), so fixed that in c564e58
Also added the suggested change.

if (SystemTables.containsTableName(tableName)) {
throw new AccumuloException("Cannot set set tablet availability for table " + tableName);
}

checkArgument(range != null, "range is null");
checkArgument(availability != null, "tabletAvailability is null");

Expand Down
4 changes: 2 additions & 2 deletions server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import static org.apache.accumulo.core.util.Validators.NOT_BUILTIN_NAMESPACE;
import static org.apache.accumulo.core.util.Validators.NOT_BUILTIN_TABLE;
import static org.apache.accumulo.core.util.Validators.NOT_BUILTIN_TABLE_ID;
import static org.apache.accumulo.core.util.Validators.NOT_METADATA_TABLE;
import static org.apache.accumulo.core.util.Validators.NOT_ROOT_TABLE_ID;
import static org.apache.accumulo.core.util.Validators.VALID_TABLE_ID;
import static org.apache.accumulo.core.util.Validators.sameNamespaceAs;
Expand Down Expand Up @@ -692,7 +691,8 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, TFateId opid, TFat
case TABLE_TABLET_AVAILABILITY: {
TableOperation tableOp = TableOperation.SET_TABLET_AVAILABILITY;
validateArgumentCount(arguments, tableOp, 3);
String tableName = validateName(arguments.get(0), tableOp, NOT_METADATA_TABLE);
String tableName =
validateName(arguments.get(0), tableOp, NOT_BUILTIN_TABLE.and(EXISTING_TABLE_NAME));
TableId tableId = null;
try {
tableId = manager.getContext().getTableId(tableName);
Expand Down