This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new d3b910c4a0 Remove redundant main from zoo-info-viewer tool (#3481)
d3b910c4a0 is described below
commit d3b910c4a0ea694ec047ac4ac98eb8ba990e95d9
Author: EdColeman <d...@etcoleman.com>
AuthorDate: Mon Jun 12 17:12:06 2023 -0400
Remove redundant main from zoo-info-viewer tool (#3481)
* remove unneeded instanceName and instanceId command line options
* improve KeywordStartIT to check for new KeywordExecutables with main
methods
* improve KeywordStartIT to behave better in IDE
* improve javadoc to KeywordExecutable for recommendation regarding main
methods
---------
Co-authored-by: Ed Coleman <edcole...@apache.org>
Co-authored-by: Christopher Tubbs <ctubb...@apache.org>
---
.../accumulo/server/conf/util/ZooInfoViewer.java | 62 ++--------------
.../server/conf/util/ZooInfoViewerTest.java | 61 ++--------------
.../accumulo/start/spi/KeywordExecutable.java | 5 +-
.../apache/accumulo/test/start/KeywordStartIT.java | 84 ++++++++++++++++------
4 files changed, 77 insertions(+), 135 deletions(-)
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooInfoViewer.java
b/server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooInfoViewer.java
index 3bbfcfac79..43aae8c8d5 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooInfoViewer.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooInfoViewer.java
@@ -53,7 +53,6 @@ import java.util.stream.Collectors;
import org.apache.accumulo.core.cli.ConfigOpts;
import org.apache.accumulo.core.clientImpl.Namespace;
-import org.apache.accumulo.core.conf.SiteConfiguration;
import org.apache.accumulo.core.data.InstanceId;
import org.apache.accumulo.core.data.NamespaceId;
import org.apache.accumulo.core.data.TableId;
@@ -101,10 +100,6 @@ public class ZooInfoViewer implements KeywordExecutable {
*/
public ZooInfoViewer() {}
- public static void main(String[] args) throws Exception {
- new ZooInfoViewer().execute(args);
- }
-
@Override
public String keyword() {
return "zoo-info-viewer";
@@ -125,10 +120,14 @@ public class ZooInfoViewer implements KeywordExecutable {
log.info("print properties: {}", opts.printProps);
log.info("print instances: {}", opts.printInstanceIds);
- ZooReader zooReader = new ZooReaderWriter(opts.getSiteConfiguration());
+ var conf = opts.getSiteConfiguration();
+
+ ZooReader zooReader = new ZooReaderWriter(conf);
- InstanceId iid = getInstanceId(zooReader, opts);
- generateReport(iid, opts, zooReader);
+ try (ServerContext context = new ServerContext(conf)) {
+ InstanceId iid = context.getInstanceID();
+ generateReport(iid, opts, zooReader);
+ }
}
void generateReport(final InstanceId iid, final ZooInfoViewer.Opts opts,
@@ -170,45 +169,6 @@ public class ZooInfoViewer implements KeywordExecutable {
}
}
- /**
- * Get the instanceID from the command line options, or from value stored in
HDFS. The search
- * order is:
- * <ol>
- * <li>command line: --instanceId option</li>
- * <li>command line: --instanceName option</li>
- * <li>HDFS</li>
- * </ol>
- *
- * @param zooReader a ZooReader
- * @param opts the parsed command line options.
- * @return an instance id
- */
- InstanceId getInstanceId(final ZooReader zooReader, final ZooInfoViewer.Opts
opts) {
-
- if (!opts.instanceId.isEmpty()) {
- return InstanceId.of(opts.instanceId);
- }
- if (!opts.instanceName.isEmpty()) {
- Map<String,InstanceId> instanceNameToIdMap =
readInstancesFromZk(zooReader);
- String instanceName = opts.instanceName;
- for (Map.Entry<String,InstanceId> e : instanceNameToIdMap.entrySet()) {
- if (e.getKey().equals(instanceName)) {
- return e.getValue();
- }
- }
- throw new IllegalArgumentException(
- "Specified instance name '" + instanceName + "' not found in
ZooKeeper");
- }
-
- try (ServerContext context = new ServerContext(SiteConfiguration.auto())) {
- return context.getInstanceID();
- } catch (Exception ex) {
- throw new IllegalArgumentException(
- "Failed to read instance id from HDFS. Instances can be specified on
the command line",
- ex);
- }
- }
-
Map<NamespaceId,String> getNamespaceIdToNameMap(InstanceId iid, final
ZooReader zooReader) {
SortedMap<NamespaceId,String> namespaceToName = new TreeMap<>();
String zooNsRoot = ZooUtil.getRoot(iid) + ZNAMESPACES;
@@ -566,14 +526,6 @@ public class ZooInfoViewer implements KeywordExecutable {
description = "print the instance ids stored in ZooKeeper")
public boolean printInstanceIds = false;
- @Parameter(names = {"--instanceName"},
- description = "Specify the instance name to use. If instance name or
id are not provided, determined from configuration (requires a running hdfs
instance)")
- public String instanceName = "";
-
- @Parameter(names = {"--instanceId"},
- description = "Specify the instance id to use. If instance name or id
are not provided, determined from configuration (requires a running hdfs
instance)")
- public String instanceId = "";
-
@Parameter(names = {"-ns", "--namespaces"},
description = "a list of namespace names to print properties, with
none specified, print all. Only valid with --print-props",
variableArity = true)
diff --git
a/server/base/src/test/java/org/apache/accumulo/server/conf/util/ZooInfoViewerTest.java
b/server/base/src/test/java/org/apache/accumulo/server/conf/util/ZooInfoViewerTest.java
index 38b09734b7..7cd89f75f0 100644
---
a/server/base/src/test/java/org/apache/accumulo/server/conf/util/ZooInfoViewerTest.java
+++
b/server/base/src/test/java/org/apache/accumulo/server/conf/util/ZooInfoViewerTest.java
@@ -159,57 +159,6 @@ public class ZooInfoViewerTest {
verify(zooReader);
}
- /**
- * Expect that instance id passed is returned, instance name and zooReader
are ignored.
- */
- @Test
- public void instanceIdOption() throws Exception {
-
- String instAName = "INST_A";
- InstanceId instA = InstanceId.of(UUID.randomUUID());
- String instBName = "INST_B";
- InstanceId instB = InstanceId.of(UUID.randomUUID());
-
- ZooReader zooReader = createMock(ZooReader.class);
- String namePath = ZROOT + ZINSTANCES;
- expect(zooReader.getChildren(eq(namePath))).andReturn(List.of(instAName,
instBName)).once();
- expect(zooReader.getData(eq(namePath + "/" + instAName)))
- .andReturn(instA.canonical().getBytes(UTF_8)).once();
- expect(zooReader.getData(eq(namePath + "/" + instBName)))
- .andReturn(instB.canonical().getBytes(UTF_8)).once();
- replay(zooReader);
-
- ZooInfoViewer.Opts opts = new ZooInfoViewer.Opts();
- opts.parseArgs(ZooInfoViewer.class.getName(), new String[]
{"--instanceName", instBName});
-
- ZooInfoViewer viewer = new ZooInfoViewer();
- InstanceId found = viewer.getInstanceId(zooReader, opts);
-
- assertEquals(instB, found);
-
- verify(zooReader);
- }
-
- /**
- *
- */
- @Test
- public void instanceNameTest() {
- String uuid = UUID.randomUUID().toString();
- ZooReader zooReader = createMock(ZooReader.class);
- ZooInfoViewer.Opts opts = new ZooInfoViewer.Opts();
- opts.parseArgs(ZooInfoViewer.class.getName(),
- new String[] {"--instanceId", uuid, "--instanceName", "foo"});
- replay(zooReader);
-
- ZooInfoViewer viewer = new ZooInfoViewer();
- InstanceId found = viewer.getInstanceId(zooReader, opts);
-
- assertEquals(InstanceId.of(uuid), found);
-
- verify(zooReader);
- }
-
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "test
generated output")
@Test
public void instanceIdOutputTest() throws Exception {
@@ -226,7 +175,7 @@ public class ZooInfoViewerTest {
ZooInfoViewer.Opts opts = new ZooInfoViewer.Opts();
opts.parseArgs(ZooInfoViewer.class.getName(),
- new String[] {"--instanceId", uuid, "--print-instances", "--outfile",
testFileName});
+ new String[] {"--print-instances", "--outfile", testFileName});
ZooInfoViewer viewer = new ZooInfoViewer();
viewer.generateReport(InstanceId.of(uuid), opts, zooReader);
@@ -262,8 +211,8 @@ public class ZooInfoViewerTest {
String testFileName = "./target/zoo-info-viewer-" +
System.currentTimeMillis() + ".txt";
ZooInfoViewer.Opts opts = new ZooInfoViewer.Opts();
- opts.parseArgs(ZooInfoViewer.class.getName(), new String[]
{"--instanceName", instanceName,
- "--print-instances", "--outfile", testFileName});
+ opts.parseArgs(ZooInfoViewer.class.getName(),
+ new String[] {"--print-instances", "--outfile", testFileName});
ZooInfoViewer viewer = new ZooInfoViewer();
viewer.generateReport(InstanceId.of(uuid), opts, zooReader);
@@ -364,7 +313,7 @@ public class ZooInfoViewerTest {
ZooInfoViewer.Opts opts = new ZooInfoViewer.Opts();
opts.parseArgs(ZooInfoViewer.class.getName(),
- new String[] {"--instanceId", uuid, "--print-props", "--outfile",
testFileName});
+ new String[] {"--print-props", "--outfile", testFileName});
ZooInfoViewer viewer = new ZooInfoViewer();
viewer.generateReport(InstanceId.of(uuid), opts, zooReader);
@@ -426,7 +375,7 @@ public class ZooInfoViewerTest {
ZooInfoViewer.Opts opts = new ZooInfoViewer.Opts();
opts.parseArgs(ZooInfoViewer.class.getName(),
- new String[] {"--instanceName", instanceName, "--print-id-map",
"--outfile", testFileName});
+ new String[] {"--print-id-map", "--outfile", testFileName});
ZooInfoViewer viewer = new ZooInfoViewer();
viewer.generateReport(InstanceId.of(uuid), opts, zooReader);
diff --git
a/start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java
b/start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java
index 65e4ac8f08..83015a77ab 100644
--- a/start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java
+++ b/start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java
@@ -37,8 +37,9 @@ import java.util.ServiceLoader;
* <a
href="https://github.com/google/auto/tree/master/service";>AutoService</a>
annotation.
*
* <p>
- * If the implementing class also wishes to have a redundant main method, it
may be useful to simply
- * implement main as:<br>
+ * It generally should be avoided, but if the implementing class also must
have a redundant main
+ * method, it may be useful to simply implement main as the following to
ensure consistency of
+ * behavior when executing the main method and when executing using the
keyword:<br>
* {@code new MyImplementingClass().execute(args);}
*/
public interface KeywordExecutable {
diff --git
a/test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
b/test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
index c17bbd386f..2887d9e846 100644
--- a/test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
+++ b/test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
@@ -18,9 +18,11 @@
*/
package org.apache.accumulo.test.start;
+import static java.util.stream.Collectors.toSet;
import static org.apache.accumulo.harness.AccumuloITBase.SUNNY_DAY;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
@@ -81,12 +83,23 @@ public class KeywordStartIT {
private final Logger log = LoggerFactory.getLogger(getClass());
+ /*
+ * Note: Tests below that use this method may be skipped if run in an IDE.
That can happen if the
+ * services files haven't been generated by the AutoService annotation
processor before this test
+ * runs. The AutoService annotation processor can be forced to run and
generate those services
+ * files by running `mvn clean package -DskipTests` before importing the
project into your IDE.
+ * There may be other ways to run annotation processors in your IDE, so this
may not be necessary,
+ * depending on your IDE and its configuration.
+ */
+ private Map<String,KeywordExecutable> getKeywordExecutables() {
+ var all = Main.getExecutables(ClassLoader.getSystemClassLoader());
+ assumeTrue(!all.isEmpty());
+ return all;
+ }
+
@Test
public void testKeywordsMatch() {
- for (Entry<String,KeywordExecutable> entry :
Main.getExecutables(getClass().getClassLoader())
- .entrySet()) {
- assertEquals(entry.getKey(), entry.getValue().keyword());
- }
+ getKeywordExecutables().forEach((k, v) -> assertEquals(k, v.keyword()));
}
@Test
@@ -109,12 +122,8 @@ public class KeywordStartIT {
* This test guards against accidental renaming or incorrect naming of the
keyword used to
* identify the service. The keyword is used to access the commands via the
command line, so
* changes are visible to users and should not be changed.
- * <p>
- * Note: this test may fail in Eclipse, if the services files haven't been
generated by the
- * AutoService annotation processor
*/
@Test
- @SuppressWarnings("deprecation")
public void testExpectedClasses() {
assumeTrue(new File(System.getProperty("user.dir") + "/src").exists());
TreeMap<String,Class<? extends KeywordExecutable>> expectSet = new
TreeMap<>();
@@ -125,6 +134,7 @@ public class KeywordStartIT {
expectSet.put("compactor", CompactorExecutable.class);
expectSet.put("config-upgrade", ConfigPropertyUpgrader.class);
expectSet.put("convert-config", ConvertConfig.class);
+ expectSet.put("create-empty", CreateEmpty.class);
expectSet.put("create-token", CreateToken.class);
expectSet.put("dump-zoo", DumpZookeeper.class);
expectSet.put("ec-admin", ECAdmin.class);
@@ -135,25 +145,26 @@ public class KeywordStartIT {
expectSet.put("init", Initialize.class);
expectSet.put("login-info", LoginProperties.class);
expectSet.put("manager", ManagerExecutable.class);
- expectSet.put("master",
org.apache.accumulo.manager.MasterExecutable.class);
expectSet.put("minicluster", MiniClusterExecutable.class);
expectSet.put("monitor", MonitorExecutable.class);
expectSet.put("rfile-info", PrintInfo.class);
- expectSet.put("wal-info", LogReader.class);
expectSet.put("shell", Shell.class);
- expectSet.put("tserver", TServerExecutable.class);
- expectSet.put("version", Version.class);
- expectSet.put("zookeeper", ZooKeeperMain.class);
- expectSet.put("create-empty", CreateEmpty.class);
expectSet.put("split-large", SplitLarge.class);
expectSet.put("sserver", ScanServerExecutable.class);
+ expectSet.put("tserver", TServerExecutable.class);
+ expectSet.put("version", Version.class);
+ expectSet.put("wal-info", LogReader.class);
expectSet.put("zoo-info-viewer", ZooInfoViewer.class);
expectSet.put("zoo-zap", ZooZap.class);
+ expectSet.put("zookeeper", ZooKeeperMain.class);
+
+ @SuppressWarnings("deprecation")
+ var masterExecutableClass =
org.apache.accumulo.manager.MasterExecutable.class;
+ expectSet.put("master", masterExecutableClass);
Iterator<Entry<String,Class<? extends KeywordExecutable>>> expectIter =
expectSet.entrySet().iterator();
- TreeMap<String,KeywordExecutable> actualSet =
- new TreeMap<>(Main.getExecutables(getClass().getClassLoader()));
+ TreeMap<String,KeywordExecutable> actualSet = new
TreeMap<>(getKeywordExecutables());
Iterator<Entry<String,KeywordExecutable>> actualIter =
actualSet.entrySet().iterator();
Entry<String,Class<? extends KeywordExecutable>> expected;
Entry<String,KeywordExecutable> actual;
@@ -179,8 +190,12 @@ public class KeywordStartIT {
assertFalse(moreActual, "Found additional unexpected classes");
}
+ /**
+ * This test validates that legacy tools that had a main method that the
main method is not
+ * removed to support user scripts that may use that main method. New
utilities should refrain
+ * from adding a main method and instead rely on the ServiceLoader
capability.
+ */
@Test
- @SuppressWarnings("deprecation")
public void checkHasMain() {
assertFalse(hasMain(this.getClass()),
"Sanity check for test failed. Somehow the test class has a main
method");
@@ -188,25 +203,50 @@ public class KeywordStartIT {
HashSet<Class<?>> expectSet = new HashSet<>();
expectSet.add(Admin.class);
expectSet.add(CheckCompactionConfig.class);
+ expectSet.add(CheckServerConfig.class);
+ expectSet.add(ConfigPropertyUpgrader.class);
+ expectSet.add(ConvertConfig.class);
+ expectSet.add(CreateEmpty.class);
expectSet.add(CreateToken.class);
expectSet.add(DumpZookeeper.class);
+ expectSet.add(ECAdmin.class);
+ expectSet.add(GenerateSplits.class);
expectSet.add(Info.class);
expectSet.add(Initialize.class);
+ expectSet.add(LogReader.class);
expectSet.add(LoginProperties.class);
- expectSet.add(org.apache.accumulo.master.Master.class);
expectSet.add(MiniAccumuloRunner.class);
expectSet.add(Monitor.class);
expectSet.add(PrintInfo.class);
- expectSet.add(LogReader.class);
expectSet.add(Shell.class);
expectSet.add(SimpleGarbageCollector.class);
+ expectSet.add(SplitLarge.class);
expectSet.add(TabletServer.class);
expectSet.add(ZooKeeperMain.class);
+ expectSet.add(ZooZap.class);
- for (Class<?> c : expectSet) {
- assertTrue(hasMain(c), "Class " + c.getName() + " is missing a main
method!");
- }
+ // not a KeywordExecutable, but this is known to have a main method
+ @SuppressWarnings("deprecation")
+ var masterClass = org.apache.accumulo.master.Master.class;
+ expectSet.add(masterClass);
+
+ // check that classes in the expected set contain a main
+ // not all have them; these do because they always have, and we don't want
to break things
+ expectSet.forEach(
+ c -> assertTrue(hasMain(c), "Class " + c.getName() + " is missing a
main method!"));
+
+ // build a list of all classed that implement KeywordExecutable
+ var all =
getKeywordExecutables().values().stream().map(Object::getClass).collect(toSet());
+
+ // remove the ones we already verified have a main method
+ assertTrue(all.removeAll(expectSet));
+
+ // ensure there's still some left (there should be some that don't have a
main method)
+ assertNotEquals(0, all.size());
+ // for those remaining, make sure they *don't* have an unexpected main
method
+ all.forEach(
+ c -> assertFalse(hasMain(c), "Class " + c.getName() + " has an
unexpected main method!"));
}
private static boolean hasMain(Class<?> classToCheck) {
