kevinrr888 commented on code in PR #5798:
URL: https://github.com/apache/accumulo/pull/5798#discussion_r2274670362
##########
core/src/main/java/org/apache/accumulo/core/fate/Fate.java:
##########
@@ -114,8 +116,8 @@ public enum FateOperation {
TABLE_TABLET_AVAILABILITY(TFateOperation.TABLE_TABLET_AVAILABILITY);
private final TFateOperation top;
- private static final Set<FateOperation> nonThriftOps =
Collections.unmodifiableSet(
- EnumSet.of(COMMIT_COMPACTION, SHUTDOWN_TSERVER, SYSTEM_SPLIT,
SYSTEM_MERGE));
+ private static final Set<FateOperation> nonThriftOps =
Arrays.stream(FateOperation.values())
+ .filter(fateOp -> fateOp.top ==
null).collect(Collectors.toUnmodifiableSet());
Review Comment:
this does not change current functionality, just more future-proof
##########
core/src/main/java/org/apache/accumulo/core/fate/FateExecutor.java:
##########
@@ -60,7 +61,9 @@
/**
* Handles finding and working on FATE work. Only finds/works on fate
operations that it is assigned
- * to work on defined by 'fateOps'
+ * to work on defined by 'fateOps'. These executors may be stopped and new
ones started throughout
+ * FATEs life, depending on changes to {@link
Property#MANAGER_FATE_USER_CONFIG} and
+ * {@link Property#MANAGER_FATE_META_CONFIG}.
Review Comment:
This functionality already exists, just improved description
##########
core/src/main/java/org/apache/accumulo/core/fate/FateExecutor.java:
##########
@@ -201,17 +208,23 @@ protected Set<Fate.FateOperation> getFateOps() {
return fateOps;
}
+ public FateExecutorMetrics<T> getFateExecutorMetrics() {
+ return fateExecutorMetrics;
+ }
+
/**
* Initiates the shutdown of this FateExecutor. This means the pool
executing TransactionRunners
* will no longer accept new TransactionRunners, the currently running
TransactionRunners will
- * terminate after they are done with their current transaction, if
applicable, and the work
- * finder is interrupted. {@link #isShutdown()} returns true after this is
called.
+ * terminate after they are done with their current transaction, if
applicable, the work finder is
+ * interrupted, and the metrics created for this FateExecutor are removed
from the registry (if
+ * metrics were enabled). {@link #isShutdown()} returns true after this is
called.
*/
protected void initiateShutdown() {
transactionExecutor.shutdown();
synchronized (runningTxRunners) {
runningTxRunners.forEach(TransactionRunner::flagStop);
}
+ fateExecutorMetrics.clearMetrics();
workFinder.interrupt();
}
Review Comment:
This will remove metrics from the registry when shutdown is initiated, not
necessarily when the threads are done with the fate operations they are working
on. Alternatively, could clear the metrics after all threads have terminated
##########
test/src/main/java/org/apache/accumulo/test/fate/SlowFateSplitManager.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.test.fate;
+
+import java.io.IOException;
+
+import org.apache.accumulo.core.cli.ConfigOpts;
+import org.apache.accumulo.core.fate.Fate;
+import org.apache.accumulo.core.fate.FateStore;
+import org.apache.accumulo.manager.Manager;
+import org.apache.accumulo.manager.tableOps.TraceRepo;
+import org.apache.accumulo.server.ServerContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * See {@link SlowFateSplit}
+ */
+public class SlowFateSplitManager extends Manager {
+ private static final Logger log =
LoggerFactory.getLogger(SlowFateSplitManager.class);
+ // causes splits to take at least 10 seconds to complete
+ public static final long SLEEP_TIME_MS = 10_000;
+ // important that this is an op that can be initiated on some system table
as well as user tables
+ public static final Fate.FateOperation SLOW_OP =
Fate.FateOperation.TABLE_SPLIT;
Review Comment:
overall, not a fan of this set sleep time and the hard set operation
(split). Ideally, would want to use a latch or have the sleep time and
operation type configurable in the test. But no easy way to do this since this
manager and the test exist in different JVMs
##########
test/src/main/java/org/apache/accumulo/test/metrics/MetricsIT.java:
##########
@@ -232,6 +258,186 @@ public void confirmMetricsPublished() throws Exception {
cluster.stop();
}
+ @Test
+ public void testFateExecutorMetrics() throws Exception {
+ // Tests metrics for Fate's thread pools. Tests that metrics are seen as
expected, and config
+ // changes to the thread pools are accurately reflected in the metrics.
This includes checking
+ // that old thread pool metrics are removed, new ones are created, size
changes to thread
+ // pools are reflected, and the ops assigned and instance type tags are
seen as expected
Review Comment:
Not a fan of how long and complex this test case is, but not much more
simplification can be done without sacrificing test coverage
##########
test/src/main/java/org/apache/accumulo/test/fate/SlowFateSplitManager.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.test.fate;
+
+import java.io.IOException;
+
+import org.apache.accumulo.core.cli.ConfigOpts;
+import org.apache.accumulo.core.fate.Fate;
+import org.apache.accumulo.core.fate.FateStore;
+import org.apache.accumulo.manager.Manager;
+import org.apache.accumulo.manager.tableOps.TraceRepo;
+import org.apache.accumulo.server.ServerContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * See {@link SlowFateSplit}
+ */
+public class SlowFateSplitManager extends Manager {
Review Comment:
This is largely modeled after the existing FlakyManager. A follow on could
move shared functionality into a new abstract class. Something like
CustomFateManager
--
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]
