keith-turner commented on code in PR #6126:
URL: https://github.com/apache/accumulo/pull/6126#discussion_r2805919611
##########
server/base/src/main/java/org/apache/accumulo/server/util/FindCompactionTmpFiles.java:
##########
@@ -50,48 +52,17 @@
import com.beust.jcommander.JCommander;
import com.beust.jcommander.Parameter;
-import com.beust.jcommander.Parameters;
import com.google.auto.service.AutoService;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.context.Scope;
+
@AutoService(KeywordExecutable.class)
-public class FindCompactionTmpFiles implements KeywordExecutable {
+public class FindCompactionTmpFiles extends ServerKeywordExecutable<FindOpts> {
Review Comment:
Not a change for this PR, but this command could probably be phased out. I
will open an issue. In 4.0 the compaction tmp files have the compaction uuid
in them and I believe they are automatically cleanup now. Maybe just need
this for pre 4.0 files? Maybe we could do something at upgrade time for that?
##########
server/base/src/main/java/org/apache/accumulo/server/util/Info.java:
##########
@@ -20,19 +20,23 @@
import java.util.Set;
+import org.apache.accumulo.core.cli.ServerOpts;
import org.apache.accumulo.core.client.admin.servers.ServerId;
-import org.apache.accumulo.core.conf.SiteConfiguration;
import org.apache.accumulo.core.util.MonitorUtil;
import org.apache.accumulo.server.ServerContext;
import org.apache.accumulo.start.spi.CommandGroup;
import org.apache.accumulo.start.spi.CommandGroups;
import org.apache.accumulo.start.spi.KeywordExecutable;
-import org.apache.zookeeper.KeeperException;
+import com.beust.jcommander.JCommander;
import com.google.auto.service.AutoService;
@AutoService(KeywordExecutable.class)
-public class Info implements KeywordExecutable {
+public class Info extends ServerKeywordExecutable<ServerOpts> {
Review Comment:
Wondering if this command could go away in favor of the service status
command. Will open an issue.
##########
core/src/main/java/org/apache/accumulo/core/cli/BaseKeywordExecutable.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.cli;
+
+import org.apache.accumulo.start.spi.KeywordExecutable;
+
+import com.beust.jcommander.JCommander;
+import com.beust.jcommander.ParameterException;
+
+public abstract class BaseKeywordExecutable<OPTS extends Help> implements
KeywordExecutable {
+
+ private final OPTS options;
+
+ protected BaseKeywordExecutable(OPTS options) {
+ this.options = options;
+ }
+
+ @Override
+ public final void execute(String[] args) throws Exception {
+ JCommander cl = new JCommander(this.options);
+ cl.setProgramName("accumulo "
+ + (commandGroup().key().isBlank() ? "" : commandGroup().key() + " ") +
keyword());
+ try {
+ cl.parse(args);
+ } catch (ParameterException e) {
+ cl.usage();
+ return;
+ }
+ this.options.validateArgs();
+ if (this.options.help) {
+ cl.usage();
+ return;
+ }
+ doExecute(cl, options);
+ }
+
+ // This method exists so that subclasses can override and perform
+ // pre and post-execute operations if needed.
+ public void doExecute(JCommander cl, OPTS options) throws Exception {
Review Comment:
Could this be protected?
##########
server/base/src/main/java/org/apache/accumulo/server/util/TableDiskUsage.java:
##########
@@ -70,7 +77,8 @@
* For more accurate information a compaction should first be run on all files
for the set of tables
* being computed.
*/
-public class TableDiskUsage {
+@AutoService(KeywordExecutable.class)
Review Comment:
The shell `du` command calls this same code indirectly. We could probably
drop the main method from this class and not add it as a KWE.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooKeeperMain.java:
##########
@@ -63,19 +63,16 @@ public String description() {
}
@Override
- public void execute(final String[] args) throws Exception {
- Opts opts = new Opts();
- opts.parseArgs(ZooKeeperMain.class.getName(), args);
- try (var context = new ServerContext(SiteConfiguration.auto())) {
- if (opts.servers == null) {
- opts.servers = context.getZooKeepers();
- }
- System.out.println("The accumulo instance id is " +
context.getInstanceID());
- if (!opts.servers.contains("/")) {
- opts.servers += ZooUtil.getRoot(context.getInstanceID());
- }
- org.apache.zookeeper.ZooKeeperMain
- .main(new String[] {"-server", opts.servers, "-timeout", "" +
(opts.timeout * 1000)});
+ public void execute(JCommander cl, ZKOpts opts) throws Exception {
Review Comment:
Not an issue w/ this PR. The existing description is misleading for this
command. Sounds like it starting a zookeeper server, but it seems like it
starts a ZK client.
##########
core/src/main/java/org/apache/accumulo/core/util/Merge.java:
##########
@@ -39,17 +40,24 @@
import org.apache.accumulo.core.metadata.SystemTables;
import org.apache.accumulo.core.metadata.schema.TabletsMetadata;
import org.apache.accumulo.core.trace.TraceUtil;
+import org.apache.accumulo.core.util.Merge.Opts;
+import org.apache.accumulo.start.spi.CommandGroup;
+import org.apache.accumulo.start.spi.CommandGroups;
+import org.apache.accumulo.start.spi.KeywordExecutable;
import org.apache.hadoop.io.Text;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.beust.jcommander.IStringConverter;
+import com.beust.jcommander.JCommander;
import com.beust.jcommander.Parameter;
+import com.google.auto.service.AutoService;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Scope;
-public class Merge {
+@AutoService(KeywordExecutable.class)
Review Comment:
This shell also offers this command and the shell uses this code. I think
we could just drop main from this class and not add it as a KWE.
##########
assemble/bin/accumulo-cluster:
##########
@@ -582,7 +582,7 @@ function control_services() {
# before zapping the nodes in ZooKeeper
wait
echo "Cleaning scan server entries from zookeeper for resource group
$group"
- debugOrRunAsync "$accumulo_cmd" org.apache.accumulo.server.util.ZooZap
-verbose -sservers --include-groups "$group"
+ debugOrRunAsync "$accumulo_cmd" other
org.apache.accumulo.server.util.ZooZap -verbose -sservers --include-groups
"$group"
Review Comment:
Looking at start.Main it seems like it will get the class name from
`args[0]` which would be `other` in this case. Will this work?
##########
server/base/src/main/java/org/apache/accumulo/server/util/RandomWriter.java:
##########
@@ -31,16 +32,23 @@
import org.apache.accumulo.core.data.Mutation;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.trace.TraceUtil;
+import org.apache.accumulo.server.util.RandomWriter.Opts;
+import org.apache.accumulo.start.spi.CommandGroup;
+import org.apache.accumulo.start.spi.CommandGroups;
+import org.apache.accumulo.start.spi.KeywordExecutable;
import org.apache.hadoop.io.Text;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.beust.jcommander.JCommander;
import com.beust.jcommander.Parameter;
+import com.google.auto.service.AutoService;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Scope;
-public class RandomWriter {
+@AutoService(KeywordExecutable.class)
+public class RandomWriter extends ClientKeywordExecutable<Opts> {
Review Comment:
Seems like this class could be moved to the test package.
--
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]
