keith-turner commented on code in PR #4133:
URL: https://github.com/apache/accumulo/pull/4133#discussion_r1449485041
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -926,9 +933,9 @@ private void writeSplitsToFile(Path splitsPath, final
List<ByteBuffer> arguments
*
* @return the path of the created directory
*/
- public Path mkTempDir(long opid) throws IOException {
+ public Path mkTempDir(TFateId opid) throws IOException {
Volume vol = manager.getVolumeManager().getFirst();
- Path p = vol.prefixChild("/tmp/fate-" + FastFormat.toHexString(opid));
+ Path p = vol.prefixChild("/tmp/fate-" +
FastFormat.toHexString(opid.getTid()));
Review Comment:
Could add the type to this tmp dir name
```suggestion
Path p = vol.prefixChild("/tmp/fate-"+opid.getType()+"-"+
FastFormat.toHexString(opid.getTid()));
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1068,17 +1073,12 @@ boolean canSuspendTablets() {
}
try {
- final AgeOffStore<Manager> store = new AgeOffStore<>(
- new org.apache.accumulo.core.fate.ZooStore<>(getZooKeeperRoot() +
Constants.ZFATE,
- context.getZooReaderWriter()),
- HOURS.toMillis(8), System::currentTimeMillis);
+ initializeFateInstance(context, FateInstanceType.META,
+ new ZooStore<>(getZooKeeperRoot() + Constants.ZFATE,
context.getZooReaderWriter()));
+ initializeFateInstance(context, FateInstanceType.USER,
+ new AccumuloStore<>(context, FateTable.NAME));
Review Comment:
After these are setup, the map never changes. Could use an immutable map
like the following.
```suggestion
var metaInstance = initializeFateInstance(context,
FateInstanceType.META,
new ZooStore<>(getZooKeeperRoot() + Constants.ZFATE,
context.getZooReaderWriter()));
var userInstance = initializeFateInstance(context,
FateInstanceType.USER,
new AccumuloStore<>(context, FateTable.NAME));
fateRefs = Map.of(FateInstanceType.META, metaInstance,
FateInstanceType.USER, userInstance);
```
and change the decleration of fateRefs to
```java
private volatile Map<FateInstanceType,Fate<Manager>> fateRefs = Map.of();
```
One downside of this that variable can not be final anymore. An upside is
its nice having the map be immutable when its never expected to change.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java:
##########
@@ -387,10 +390,12 @@ String doFateOperation(FateOperation op, List<ByteBuffer>
args, Map<String,Strin
String tableOrNamespaceName, boolean wait)
throws AccumuloSecurityException, TableExistsException,
TableNotFoundException,
AccumuloException, NamespaceExistsException, NamespaceNotFoundException {
- Long opid = null;
+ TFateId opid = null;
try {
- opid = beginFateOperation();
+ TFateInstanceType t =
tableOrNamespaceName.startsWith(Namespace.ACCUMULO.name())
+ ? TFateInstanceType.META : TFateInstanceType.USER;
+ opid = beginFateOperation(t);
Review Comment:
If we add a `toThrift()` method to FateInstanceType could do :
```suggestion
opid =
beginFateOperation(FateInstanceType.toType(tableOrNamespaceName).toThrift());
```
Could do the following for the toThrift() method.
```java
public enum FateInstanceType {
public TFateInstanceType toThrift(){
switch (this) {
case USER:
return TFateInstanceType.USER;
case META:
return TFateInstanceType.META;
default:
throw new IllegalStateException("Unknown type "+this);
}
}
}
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -938,15 +945,18 @@ public Path mkTempDir(long opid) throws IOException {
}
@Override
- public boolean cancelFateOperation(TInfo tinfo, TCredentials credentials,
long opid)
+ public boolean cancelFateOperation(TInfo tinfo, TCredentials credentials,
TFateId opid)
throws ThriftSecurityException, ThriftNotActiveServiceException {
if (!manager.security.canPerformSystemActions(credentials)) {
throw new ThriftSecurityException(credentials.getPrincipal(),
SecurityErrorCode.PERMISSION_DENIED);
}
- return manager.fate().cancel(opid);
+ return manager.fate(getType(opid)).cancel(opid.getTid());
}
+ private static FateInstanceType getType(TFateId opid) {
+ return FateInstanceType.valueOf(opid.getType().name());
Review Comment:
```suggestion
return FateInstanceType.fromThrift(opid.getType());
```
could add the following the the FateInstanceType enum and call as above.
The impl below uses switch and avoids the string valueOf lookup, may be a bit
more efficient.
```java
public static FateInstanceType fromThrift(TFateInstanceType tfit) {
switch (tfit) {
case USER:
return FateInstanceType.USER;
case META:
return FateInstanceType.META;
default:
throw new IllegalStateException("Unknown type "+tfit);
}
}
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -427,7 +433,7 @@ public void executeFateOperation(TInfo tinfo, TCredentials
c, long opid, FateOpe
}
goalMessage += "Offline table " + tableId;
- manager.fate().seedTransaction(op.toString(), opid,
+ manager.fate(type).seedTransaction(op.toString(), tid,
Review Comment:
Alot of the case statements have validation at the beggining. We could add
validation that checks the the actual table id and fate type are sensible. I
think this would be best done in a follow on PR.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java:
##########
@@ -277,12 +279,13 @@ public void create(String tableName,
NewTableConfiguration ntc)
}
}
- private long beginFateOperation() throws ThriftSecurityException, TException
{
+ private TFateId beginFateOperation(TFateInstanceType type)
Review Comment:
I was really surprised by how minimal the changes were to this class.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1178,6 +1178,17 @@ boolean canSuspendTablets() {
log.info("exiting");
}
+ private void initializeFateInstance(ServerContext context, FateInstanceType
type,
+ FateStore<Manager> store) {
+ final AgeOffStore<Manager> ageOffStore =
+ new AgeOffStore<>(store, HOURS.toMillis(8), System::currentTimeMillis);
+
+ fateRefs.put(type, new Fate<>(this, ageOffStore, TraceRepo::toLogString,
getConfiguration()));
Review Comment:
There was another comment about making the map immutable. If that approach
is taken then this comment can be ignored.
```suggestion
var prev = fateRefs.put(type, new Fate<>(this, ageOffStore,
TraceRepo::toLogString, getConfiguration()));
Preconditions.checkState(prev == null, "Unexpected previous fate
instance for type : %s", type);
```
##########
test/src/main/java/org/apache/accumulo/test/functional/SplitIT.java:
##########
@@ -92,6 +93,7 @@ protected Duration defaultTimeout() {
@Override
public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration
hadoopCoreSite) {
cfg.setProperty(Property.TSERV_MAXMEM, "5K");
+ cfg.setMemory(ServerType.TABLET_SERVER, 384, MemoryUnit.MEGABYTE);
Review Comment:
Wonder if this is needed because ITs are now writing more data to the new
fate table. May be good to open a follow on issue to track down the cause of
this.
##########
core/src/main/java/org/apache/accumulo/core/fate/FateInstanceType.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.core.fate;
+
+import org.apache.accumulo.core.clientImpl.Namespace;
+
+public enum FateInstanceType {
+ META, USER;
+
+ public static FateInstanceType toType(String tableOrNamespaceName) {
Review Comment:
Its long, but using the following for the method name may read better.
```suggestion
public static FateInstanceType fromNamespaceOrTableName(String
tableOrNamespaceName) {
```
--
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]
