ctubbsii commented on code in PR #5944: URL: https://github.com/apache/accumulo/pull/5944#discussion_r2392742270
########## server/manager/src/main/java/org/apache/accumulo/manager/tableOps/FateEnv.java: ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.manager.tableOps; + +import java.util.Set; +import java.util.concurrent.ExecutorService; + +import org.apache.accumulo.core.lock.ServiceLock; +import org.apache.accumulo.core.manager.thrift.BulkImportState; +import org.apache.accumulo.core.metadata.TServerInstance; +import org.apache.accumulo.core.metadata.schema.ExternalCompactionId; +import org.apache.accumulo.core.util.time.SteadyTime; +import org.apache.accumulo.manager.Events; +import org.apache.accumulo.manager.split.Splitter; +import org.apache.accumulo.server.ServerContext; +import org.apache.accumulo.server.fs.VolumeManager; +import org.apache.accumulo.server.tables.TableManager; + +public interface FateEnv { + ServerContext getContext(); + + Events getEvents(); + + void recordCompletion(ExternalCompactionId ecid); Review Comment: This API is very specific to compactions, but the name of the method isn't clear (it kinda reads as "record completion of this fate operation" rather than "record completion of a compaction"). The name could be improved... but I don't know what's best. Maybe this? ```suggestion void recordCompletedCompaction(ExternalCompactionId ecid); ``` ########## server/manager/src/main/java/org/apache/accumulo/manager/Manager.java: ########## @@ -571,6 +564,11 @@ public CompactionCoordinator getCompactionCoordinator() { return compactionCoordinator; } + @Override + public void recordCompletion(ExternalCompactionId ecid) { + getCompactionCoordinator().recordCompletion(ecid); + } Review Comment: 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. ########## server/manager/src/main/java/org/apache/accumulo/manager/Manager.java: ########## @@ -1525,6 +1525,12 @@ public EventCoordinator getEventCoordinator() { return nextEvent; } + @Override + public Events getEvents() { + return nextEvent; + } Review Comment: This reads like it returns a list of events, but it doesn't. Maybe this should return an EventManager? ########## server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java: ########## @@ -714,6 +716,15 @@ public TDelegationToken getDelegationToken(TInfo tinfo, TCredentials credentials } } + public static void mustBeOnline(ServerContext context, final TableId tableId) Review Comment: Does this still need to be public now that it was moved? ########## server/manager/src/main/java/org/apache/accumulo/manager/tableOps/AbstractRepo.java: ########## @@ -20,19 +20,18 @@ import org.apache.accumulo.core.fate.FateId; import org.apache.accumulo.core.fate.Repo; -import org.apache.accumulo.manager.Manager; -public abstract class ManagerRepo implements Repo<Manager> { +public abstract class AbstractRepo implements Repo<FateEnv> { Review Comment: 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. -- 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]
