Skip to content
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

Fix src and dest namespace handling in clone table operation #5334

Open
wants to merge 4 commits into
base: 2.1
Choose a base branch
from
Open

Fix src and dest namespace handling in clone table operation #5334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0047e02
Fix src and dest namespace handling in clone table operation
dlmarion Feb 13, 2025
1adde6d
Addressed PR comments
dlmarion Feb 14, 2025
3d91c31
Fix misspelling in test method names
dlmarion Feb 14, 2025
3cfb06c
Updated with PR suggestions
dlmarion Feb 14, 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
19 changes: 14 additions & 5 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 @@ -286,20 +286,29 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
keepOffline = Boolean.parseBoolean(ByteBufferUtil.toString(arguments.get(2)));
}

NamespaceId srcNamespaceId;
try {
srcNamespaceId = manager.getContext().getNamespaceId(srcTableId);
} catch (TableNotFoundException e) {
// could happen if the table was deleted while processing this request
throw new ThriftTableOperationException(srcTableId.canonical(), null, tableOp,
TableOperationExceptionType.NOTFOUND, "");
}

NamespaceId namespaceId;
try {
namespaceId = Namespaces.getNamespaceId(manager.getContext(),
TableNameUtil.qualify(tableName).getFirst());
} catch (NamespaceNotFoundException e) {
// shouldn't happen, but possible once cloning between namespaces is supported
// dest namespace does not exist yet, needs to be created
throw new ThriftTableOperationException(null, tableName, tableOp,
TableOperationExceptionType.NAMESPACE_NOTFOUND, "");
}

final boolean canCloneTable;
try {
canCloneTable =
manager.security.canCloneTable(c, srcTableId, tableName, namespaceId, namespaceId);
manager.security.canCloneTable(c, srcTableId, tableName, namespaceId, srcNamespaceId);
} catch (ThriftSecurityException e) {
throwIfTableMissingSecurityException(e, srcTableId, null, TableOperation.CLONE);
throw e;
Expand Down Expand Up @@ -336,9 +345,9 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
goalMessage += " and keep offline.";
}

manager.fate().seedTransaction(
op.toString(), opid, new TraceRepo<>(new CloneTable(c.getPrincipal(), namespaceId,
srcTableId, tableName, propertiesToSet, propertiesToExclude, keepOffline)),
manager.fate().seedTransaction(op.toString(), opid,
new TraceRepo<>(new CloneTable(c.getPrincipal(), srcNamespaceId, srcTableId,
namespaceId, tableName, propertiesToSet, propertiesToExclude, keepOffline)),
autoCleanup, goalMessage);

break;
Expand Down
79 changes: 69 additions & 10 deletions server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,73 @@ class CloneInfo implements Serializable {

private static final long serialVersionUID = 1L;

TableId srcTableId;
String tableName;
TableId tableId;
NamespaceId namespaceId;
NamespaceId srcNamespaceId;
Map<String,String> propertiesToSet;
Set<String> propertiesToExclude;
boolean keepOffline;

public String user;
private final TableId srcTableId;
private final String tableName;
private TableId tableId;
// TODO: Make final in 3.1
private NamespaceId namespaceId;
private final NamespaceId srcNamespaceId;
private final Map<String,String> propertiesToSet;
private final Set<String> propertiesToExclude;
private final boolean keepOffline;
private final String user;

public CloneInfo(NamespaceId srcNamespaceId, TableId srcTableId, NamespaceId dstNamespaceId,
String dstTableName, Map<String,String> propertiesToSet, Set<String> propertiesToExclude,
boolean keepOffline, String user) {
super();
this.srcNamespaceId = srcNamespaceId;
this.srcTableId = srcTableId;
this.tableName = dstTableName;
this.namespaceId = dstNamespaceId;
this.propertiesToSet = propertiesToSet;
this.propertiesToExclude = propertiesToExclude;
this.keepOffline = keepOffline;
this.user = user;
}

public TableId getSrcTableId() {
return srcTableId;
}

public String getTableName() {
return tableName;
}

public void setTableId(TableId dstTableId) {
this.tableId = dstTableId;
}

public TableId getTableId() {
return tableId;
}

public NamespaceId getNamespaceId() {
return namespaceId;
}

public void setNamespaceId(NamespaceId nid) {
this.namespaceId = nid;
}

public NamespaceId getSrcNamespaceId() {
return srcNamespaceId;
}

public Map<String,String> getPropertiesToSet() {
return propertiesToSet;
}

public Set<String> getPropertiesToExclude() {
return propertiesToExclude;
}

public boolean isKeepOffline() {
return keepOffline;
}

public String getUser() {
return user;
}

}
11 changes: 6 additions & 5 deletions server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,20 @@ public long isReady(long tid, Manager environment) {
@Override
public Repo<Manager> call(long tid, Manager environment) throws Exception {
LoggerFactory.getLogger(CloneMetadata.class)
.info(String.format("Cloning %s with tableId %s from srcTableId %s", cloneInfo.tableName,
cloneInfo.tableId, cloneInfo.srcTableId));
.info(String.format("Cloning %s with tableId %s from srcTableId %s",
cloneInfo.getTableName(), cloneInfo.getTableId(), cloneInfo.getSrcTableId()));
// need to clear out any metadata entries for tableId just in case this
// died before and is executing again
MetadataTableUtil.deleteTable(cloneInfo.tableId, false, environment.getContext(),
MetadataTableUtil.deleteTable(cloneInfo.getTableId(), false, environment.getContext(),
environment.getManagerLock());
MetadataTableUtil.cloneTable(environment.getContext(), cloneInfo.srcTableId, cloneInfo.tableId);
MetadataTableUtil.cloneTable(environment.getContext(), cloneInfo.getSrcTableId(),
cloneInfo.getTableId());
return new FinishCloneTable(cloneInfo);
}

@Override
public void undo(long tid, Manager environment) throws Exception {
MetadataTableUtil.deleteTable(cloneInfo.tableId, false, environment.getContext(),
MetadataTableUtil.deleteTable(cloneInfo.getTableId(), false, environment.getContext(),
environment.getManagerLock());
}

Expand Down
8 changes: 4 additions & 4 deletions ...er/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/ClonePermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {
for (TablePermission permission : TablePermission.values()) {
try {
environment.getContext().getSecurityOperation().grantTablePermission(
environment.getContext().rpcCreds(), cloneInfo.user, cloneInfo.tableId,
cloneInfo.tableName, permission, cloneInfo.namespaceId);
environment.getContext().rpcCreds(), cloneInfo.getUser(), cloneInfo.getTableId(),
cloneInfo.getTableName(), permission, cloneInfo.getNamespaceId());
} catch (ThriftSecurityException e) {
LoggerFactory.getLogger(ClonePermissions.class).error("{}", e.getMessage(), e);
throw e;
Expand All @@ -64,7 +64,7 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {
try {
return new CloneZookeeper(cloneInfo, environment.getContext());
} catch (NamespaceNotFoundException e) {
throw new AcceptableThriftTableOperationException(null, cloneInfo.tableName,
throw new AcceptableThriftTableOperationException(null, cloneInfo.getTableName(),
TableOperation.CLONE, TableOperationExceptionType.NAMESPACE_NOTFOUND,
"Namespace for target table not found");
}
Expand All @@ -73,6 +73,6 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {
@Override
public void undo(long tid, Manager environment) throws Exception {
environment.getContext().getSecurityOperation().deleteTable(environment.getContext().rpcCreds(),
cloneInfo.tableId, cloneInfo.namespaceId);
cloneInfo.getTableId(), cloneInfo.getNamespaceId());
}
}
28 changes: 11 additions & 17 deletions server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment on lines +46 to +48
Copy link
Member

Choose a reason for hiding this comment

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

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.

TableOperation.CLONE);
return val;
}
Expand All @@ -60,9 +55,8 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {

Utils.getIdLock().lock();
try {
cloneInfo.tableId =
Utils.getNextId(cloneInfo.tableName, environment.getContext(), TableId::of);

cloneInfo.setTableId(
Utils.getNextId(cloneInfo.getTableName(), environment.getContext(), TableId::of));
return new ClonePermissions(cloneInfo);
} finally {
Utils.getIdLock().unlock();
Expand All @@ -71,8 +65,8 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {

@Override
public void undo(long tid, Manager environment) {
Utils.unreserveNamespace(environment, cloneInfo.srcNamespaceId, tid, false);
Utils.unreserveTable(environment, cloneInfo.srcTableId, tid, false);
Utils.unreserveNamespace(environment, cloneInfo.getNamespaceId(), tid, false);
Utils.unreserveTable(environment, cloneInfo.getSrcTableId(), tid, false);
}

}
41 changes: 21 additions & 20 deletions server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneZookeeper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,24 @@ class CloneZookeeper extends ManagerRepo {
public CloneZookeeper(CloneInfo cloneInfo, ClientContext context)
throws NamespaceNotFoundException {
this.cloneInfo = cloneInfo;
this.cloneInfo.namespaceId = Namespaces.getNamespaceId(context,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so? namespaceId is resolved in FateServiceHandler, passed to CloneTable and set in CloneInfo there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, are you saying that this may cause pre-2.1.4 Clone fate operations to fail, and that I should still check and set this if it's null in CloneInfo?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure exactly what to expect when the serialization of the fate operation's classes are changed, but if a clonetable operation was serialized with 2.1.3, and then interrupted, and then resumed after upgrading to 2.1.4, the deserialization in order to resume the operation will probably fail in some way. I'm not sure if you need to do anything to check it in the code... maybe just warn users that they should make sure there's no outstanding clone operations before doing an upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a change in 3cfb06c to handle the case of pre-2.1.4 fate transactions with a note to remove that handling in the 3.1 merge.

TableNameUtil.qualify(this.cloneInfo.tableName).getFirst());
if (this.cloneInfo.getNamespaceId() == null) {
// Prior to 2.1.4 the namespaceId was calculated in this
// step and set on the cloneInfo object here. If for some
// reason we are processing a pre-2.1.3 CloneTable operation,
// then we need to continue to set this here as it will be
// null in the deserialized CloneInfo object.
//
// TODO: Remove this check in 3.1 as Fate operations
// need to be cleaned up before a major upgrade.
this.cloneInfo.setNamespaceId(Namespaces.getNamespaceId(context,
TableNameUtil.qualify(this.cloneInfo.getTableName()).getFirst()));
}
}

@Override
public long isReady(long tid, Manager environment) throws Exception {
long val = 0;
if (!cloneInfo.srcNamespaceId.equals(cloneInfo.namespaceId)) {
val += Utils.reserveNamespace(environment, cloneInfo.namespaceId, tid, false, true,
TableOperation.CLONE);
}
val +=
Utils.reserveTable(environment, cloneInfo.tableId, tid, true, false, TableOperation.CLONE);
return val;
return Utils.reserveTable(environment, cloneInfo.getTableId(), tid, true, false,
TableOperation.CLONE);
}

@Override
Expand All @@ -59,12 +63,12 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {
try {
// write tableName & tableId to zookeeper

Utils.checkTableNameDoesNotExist(environment.getContext(), cloneInfo.tableName,
cloneInfo.namespaceId, cloneInfo.tableId, TableOperation.CLONE);
Utils.checkTableNameDoesNotExist(environment.getContext(), cloneInfo.getTableName(),
cloneInfo.getNamespaceId(), cloneInfo.getTableId(), TableOperation.CLONE);

environment.getTableManager().cloneTable(cloneInfo.srcTableId, cloneInfo.tableId,
cloneInfo.tableName, cloneInfo.namespaceId, cloneInfo.propertiesToSet,
cloneInfo.propertiesToExclude);
environment.getTableManager().cloneTable(cloneInfo.getSrcTableId(), cloneInfo.getTableId(),
cloneInfo.getTableName(), cloneInfo.getNamespaceId(), cloneInfo.getPropertiesToSet(),
cloneInfo.getPropertiesToExclude());
environment.getContext().clearTableListCache();

return new CloneMetadata(cloneInfo);
Expand All @@ -75,11 +79,8 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {

@Override
public void undo(long tid, Manager environment) throws Exception {
environment.getTableManager().removeTable(cloneInfo.tableId);
if (!cloneInfo.srcNamespaceId.equals(cloneInfo.namespaceId)) {
Utils.unreserveNamespace(environment, cloneInfo.namespaceId, tid, false);
}
Utils.unreserveTable(environment, cloneInfo.tableId, tid, true);
environment.getTableManager().removeTable(cloneInfo.getTableId());
Utils.unreserveTable(environment, cloneInfo.getTableId(), tid, true);
environment.getContext().clearTableListCache();
}

Expand Down
25 changes: 11 additions & 14 deletions ...er/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.unreserveTable(environment, cloneInfo.getTableId(), tid, true);
Utils.unreserveNamespace(environment, cloneInfo.getNamespaceId(), tid, false);
Utils.unreserveTable(environment, cloneInfo.getSrcTableId(), tid, false);

Utils.unreserveNamespace(environment, cloneInfo.srcNamespaceId, tid, false);
if (!cloneInfo.srcNamespaceId.equals(cloneInfo.namespaceId)) {
Utils.unreserveNamespace(environment, cloneInfo.namespaceId, tid, false);
}
Utils.unreserveTable(environment, cloneInfo.srcTableId, tid, false);
Utils.unreserveTable(environment, cloneInfo.tableId, tid, true);

environment.getEventCoordinator().event("Cloned table %s from %s", cloneInfo.tableName,
cloneInfo.srcTableId);
environment.getEventCoordinator().event("Cloned table %s from %s", cloneInfo.getTableName(),
cloneInfo.getSrcTableId());

LoggerFactory.getLogger(FinishCloneTable.class).debug("Cloned table " + cloneInfo.srcTableId
+ " " + cloneInfo.tableId + " " + cloneInfo.tableName);
LoggerFactory.getLogger(FinishCloneTable.class)
.debug("Cloned table " + cloneInfo.getSrcTableId() + " " + cloneInfo.getTableId() + " "
+ cloneInfo.getTableName());

return null;
}
Expand Down
Loading