replaces Manager with new FateEnv interface in fate code #5944

keith-turner · 2025-09-30T19:07:29Z

Added a FateEnv interface that replaces direct usage of the Manager type in fate operations. This refactoring could support changes to run fate operations outside of the Manager process.

server/manager/src/main/java/org/apache/accumulo/manager/tableOps/FateEnv.java

ctubbsii · 2025-09-30T20:26:09Z

server/manager/src/main/java/org/apache/accumulo/manager/Manager.java

+  @Override
+  public void recordCompletion(ExternalCompactionId ecid) {
+    getCompactionCoordinator().recordCompletion(ecid);
+  }


This method breaks the pattern in FateEnv. Most of the other methods get a management utility, like the VolumeManager, the Splitter, the ServiceLock, etc. To follow the pattern, I'd imagine FateEnv would return a getCompactionCoordinator, instead of recording a completion directly.

I was thinking ahead here to running fate outside of the manager, in that situation accessing the entire compaction coordinator would not be possible. Would need to be some subset of it and this was all fate needed so I just made that single method available for now.

VolumeManager, the Splitter, the ServiceLock

Those things would likely be easily available in another process. One thing I noticed is the manager has a few methods for things that it just gets from the server context. Wanted to inline those, but there were already so many changes.

This is an example of a method I wanted to inline

accumulo/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java

Lines 1528 to 1530 in 1168832

public VolumeManager getVolumeManager() {

return getContext().getVolumeManager();

}

ctubbsii · 2025-09-30T20:27:11Z

server/manager/src/main/java/org/apache/accumulo/manager/Manager.java

+  @Override
+  public Events getEvents() {
+    return nextEvent;
+  }


This reads like it returns a list of events, but it doesn't. Maybe this should return an EventManager?

Maybe I will rename it to EventPublisher. There is code in the manager for publishing and subscribing to events that is all in memory, the fate code only publishes events so I wanted to only offer that functionality to fate. Thinking ahead that could be RPCs to the manager if fate is running outside the manager.

EventPublisher is a good name

renamed in b6905d7

renamed the getter in d38f060

server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java

ctubbsii · 2025-09-30T20:36:46Z

server/manager/src/main/java/org/apache/accumulo/manager/tableOps/AbstractRepo.java

-import org.apache.accumulo.manager.Manager;

-public abstract class ManagerRepo implements Repo<Manager> {
+public abstract class AbstractRepo implements Repo<FateEnv> {


I think this could be further simplified, because by making Repo use a more generic type, FateEnv, it isn't really necessary to have Repo accept a generic parameter... it can just always be FateEnv.

Also, the name "Repo" has always been a bit of a confusing name. Since you're renaming, you could take the opportunity to rename this to something more like "AbstractFateOperation" or "AbstractFateOp", since all the subclasses of this are typically referred to as "fate ops" (hence package names like "tableOps").

Also, this is still in the tableOps package, but it is being used by tserverOps, coordinator ops, etc. It probably needs a new home.

I think this could be further simplified, because by making Repo use a more generic type, FateEnv, it isn't really necessary to have Repo accept a generic parameter... it can just always be FateEnv.

Not currently, fate test do use other type parameters in order to make testing simpler. That would be a big change best for another PR, if its even feasible.

renamed in d5a63a8

kevinrr888 · 2025-10-09T16:33:32Z

server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/PrepBulkImport.java

+    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);


Suggested change

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);

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);

kevinrr888 · 2025-10-09T16:35:16Z

server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/PrepBulkImport.java

+      int skip =
+          ctx.getTableConfiguration(bulkInfo.tableId).getCount(Property.TABLE_BULK_SKIP_THRESHOLD);


Suggested change

int skip =

ctx.getTableConfiguration(bulkInfo.tableId).getCount(Property.TABLE_BULK_SKIP_THRESHOLD);

int skip =

tableConfig.getCount(Property.TABLE_BULK_SKIP_THRESHOLD);

if this is in scope of my other suggestion

kevinrr888 · 2025-10-09T16:55:59Z

server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/DeleteRows.java

  }

-  private Optional<KeyExtent> deleteTabletFiles(Manager manager, FateId fateId) {
+  private Optional<KeyExtent> deleteTabletFiles(ServerContext ctx, FateId fateId) {


Suggested change

private Optional<KeyExtent> deleteTabletFiles(ServerContext ctx, FateId fateId) {

private Optional<KeyExtent> deleteTabletFiles(Ample ample, FateId fateId) {

can pass even less

kevinrr888 · 2025-10-09T17:07:22Z

server/manager/src/main/java/org/apache/accumulo/manager/tableOps/rename/RenameTable.java

+      Utils.unreserveTable(env.getContext(), tableId, fateId, LockType.WRITE);
+      Utils.unreserveNamespace(env.getContext(), namespaceId, fateId, LockType.READ);


Suggested change

Utils.unreserveTable(env.getContext(), tableId, fateId, LockType.WRITE);

Utils.unreserveNamespace(env.getContext(), namespaceId, fateId, LockType.READ);

Utils.unreserveTable(context, tableId, fateId, LockType.WRITE);

Utils.unreserveNamespace(context, namespaceId, fateId, LockType.READ);

kevinrr888 · 2025-10-09T17:10:50Z

server/manager/src/main/java/org/apache/accumulo/manager/tableOps/split/PreSplit.java


  @Override
-  public void undo(FateId fateId, Manager manager) throws Exception {}
+  public void undo(FateId fateId, FateEnv manager) throws Exception {}


Suggested change

public void undo(FateId fateId, FateEnv manager) throws Exception {}

public void undo(FateId fateId, FateEnv env) throws Exception {}

kevinrr888 · 2025-10-09T17:37:19Z

server/manager/src/main/java/org/apache/accumulo/manager/tserverOps/ShutdownTServer.java


  @Override
-  public void undo(FateId fateId, Manager m) {}
+  public void undo(FateId fateId, FateEnv m) {}


Suggested change

public void undo(FateId fateId, FateEnv m) {}

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.

replaces Manager with new FateEnv interface in fate code

6f2f125

Added a FateEnv interface that replaces direct usage of the Manager type in fate operations. This refactoring could support changes to run fate operations outside of the Manager process.

keith-turner added this to the 4.0.0 milestone Sep 30, 2025

keith-turner added 3 commits September 30, 2025 19:12

do TODO

7776312

add override annotations

ae309c9

add override annotations

b899a83

ctubbsii reviewed Sep 30, 2025

View reviewed changes

keith-turner added 3 commits September 30, 2025 21:00

code review update

d5a63a8

code review update

b6905d7

rename method

d38f060

kevinrr888 approved these changes Oct 9, 2025

View reviewed changes

	public VolumeManager getVolumeManager() {
	return getContext().getVolumeManager();
	}

		int skip =
		ctx.getTableConfiguration(bulkInfo.tableId).getCount(Property.TABLE_BULK_SKIP_THRESHOLD);

	private Optional<KeyExtent> deleteTabletFiles(ServerContext ctx, FateId fateId) {
	private Optional<KeyExtent> deleteTabletFiles(Ample ample, FateId fateId) {

		Utils.unreserveTable(env.getContext(), tableId, fateId, LockType.WRITE);
		Utils.unreserveNamespace(env.getContext(), namespaceId, fateId, LockType.READ);

	public void undo(FateId fateId, FateEnv manager) throws Exception {}
	public void undo(FateId fateId, FateEnv env) throws Exception {}

replaces Manager with new FateEnv interface in fate code #5944

Are you sure you want to change the base?

replaces Manager with new FateEnv interface in fate code #5944

Uh oh!

Conversation

keith-turner commented Sep 30, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

keith-turner Sep 30, 2025 •

edited

Loading