kevinrr888 commented on code in PR #5944:
URL: https://github.com/apache/accumulo/pull/5944#discussion_r2417337901
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/PrepBulkImport.java:
##########
@@ -220,15 +220,15 @@ private static void checkFilesPerTablet(String tableId,
int maxFilesPerTablet,
}
}
- private void checkForMerge(final Manager manager, final FateId fateId)
throws Exception {
+ private void checkForMerge(final ServerContext ctx, final FateId fateId)
throws Exception {
- VolumeManager fs = manager.getVolumeManager();
+ VolumeManager fs = ctx.getVolumeManager();
final Path bulkDir = new Path(bulkInfo.sourceDir);
- int maxTablets =
manager.getContext().getTableConfiguration(bulkInfo.tableId)
- .getCount(Property.TABLE_BULK_MAX_TABLETS);
- int maxFilesPerTablet =
manager.getContext().getTableConfiguration(bulkInfo.tableId)
- .getCount(Property.TABLE_BULK_MAX_TABLET_FILES);
+ int maxTablets =
+
ctx.getTableConfiguration(bulkInfo.tableId).getCount(Property.TABLE_BULK_MAX_TABLETS);
+ int maxFilesPerTablet =
+
ctx.getTableConfiguration(bulkInfo.tableId).getCount(Property.TABLE_BULK_MAX_TABLET_FILES);
Review Comment:
```suggestion
var tableConfig = ctx.getTableConfiguration(bulkInfo.tableId);
int maxTablets =
tableConfig.getCount(Property.TABLE_BULK_MAX_TABLETS);
int maxFilesPerTablet =
tableConfig.getCount(Property.TABLE_BULK_MAX_TABLET_FILES);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/PrepBulkImport.java:
##########
@@ -252,24 +252,24 @@ public void close() {
}
};
- int skip = manager.getContext().getTableConfiguration(bulkInfo.tableId)
- .getCount(Property.TABLE_BULK_SKIP_THRESHOLD);
+ int skip =
+
ctx.getTableConfiguration(bulkInfo.tableId).getCount(Property.TABLE_BULK_SKIP_THRESHOLD);
Review Comment:
```suggestion
int skip =
tableConfig.getCount(Property.TABLE_BULK_SKIP_THRESHOLD);
```
if this is in scope of my other suggestion
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/split/PreSplit.java:
##########
@@ -152,5 +152,5 @@ public Repo<Manager> call(FateId fateId, Manager manager)
throws Exception {
}
@Override
- public void undo(FateId fateId, Manager manager) throws Exception {}
+ public void undo(FateId fateId, FateEnv manager) throws Exception {}
Review Comment:
```suggestion
public void undo(FateId fateId, FateEnv env) throws Exception {}
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/rename/RenameTable.java:
##########
@@ -57,20 +57,20 @@ public RenameTable(NamespaceId namespaceId, TableId
tableId, String oldTableName
}
@Override
- public Repo<Manager> call(FateId fateId, Manager manager) throws Exception {
+ public Repo<FateEnv> call(FateId fateId, FateEnv env) throws Exception {
Pair<String,String> qualifiedOldTableName =
TableNameUtil.qualify(oldTableName);
// the namespace name was already checked before starting the fate
operation
String newSimpleTableName =
TableNameUtil.qualify(newTableName).getSecond();
- var context = manager.getContext();
+ var context = env.getContext();
try {
context.getTableMapping(namespaceId).rename(tableId,
qualifiedOldTableName.getSecond(),
newSimpleTableName);
context.clearTableListCache();
} finally {
- Utils.unreserveTable(manager.getContext(), tableId, fateId,
LockType.WRITE);
- Utils.unreserveNamespace(manager.getContext(), namespaceId, fateId,
LockType.READ);
+ Utils.unreserveTable(env.getContext(), tableId, fateId, LockType.WRITE);
+ Utils.unreserveNamespace(env.getContext(), namespaceId, fateId,
LockType.READ);
Review Comment:
```suggestion
Utils.unreserveTable(context, tableId, fateId, LockType.WRITE);
Utils.unreserveNamespace(context, namespaceId, fateId, LockType.READ);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tserverOps/ShutdownTServer.java:
##########
@@ -112,5 +114,5 @@ public Repo<Manager> call(FateId fateId, Manager manager)
throws Exception {
}
@Override
- public void undo(FateId fateId, Manager m) {}
+ public void undo(FateId fateId, FateEnv m) {}
Review Comment:
```suggestion
public void undo(FateId fateId, FateEnv env) {}
```
Could grep for `FateEnv m` to see if there are other occurrences of
something similar
Not for this PR but, could also just delete this method (and others that
don't do anything for undo) since default impl does nothing already.
Alternatively, might be better to make undo (maybe isReady too) in
AbstractFateOperation abstract methods so implementers have to actively
consider what isReady and undo should actually do.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/DeleteRows.java:
##########
@@ -69,25 +70,24 @@ public DeleteRows(MergeInfo data) {
}
@Override
- public Repo<Manager> call(FateId fateId, Manager manager) throws Exception {
+ public Repo<FateEnv> call(FateId fateId, FateEnv env) throws Exception {
// delete or fence files within the deletion range
- var mergeRange = deleteTabletFiles(manager, fateId);
+ var mergeRange = deleteTabletFiles(env.getContext(), fateId);
// merge away empty tablets in the deletion range
return new MergeTablets(mergeRange.map(mre ->
data.useMergeRange(mre)).orElse(data));
}
- private Optional<KeyExtent> deleteTabletFiles(Manager manager, FateId
fateId) {
+ private Optional<KeyExtent> deleteTabletFiles(ServerContext ctx, FateId
fateId) {
Review Comment:
```suggestion
private Optional<KeyExtent> deleteTabletFiles(Ample ample, FateId fateId) {
```
can pass even less
--
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]
