You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2026/02/10 19:46:15 UTC
[PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
dlmarion opened a new pull request, #6121:
URL: https://github.com/apache/accumulo/pull/6121
This change does the following:
* Removes the UsageGroup enum
* Adds a UsageGroups class that uses a ServiceLoader to load UsageGroup definitions
* Converted all uses of UsageGroup enum to UsageGroups static variables
* Adds a ClientUsageGroup for client utilities
* Adds a ClientKeywordExecutable object
* Modifies Help, Version, and Shell to extend ClientKeywordExecutable
* Modifies more server utilities to extend ServerKeywordExecutable
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#discussion_r2795681158
##########
start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java:
##########
@@ -44,10 +44,6 @@
*/
public interface KeywordExecutable {
- enum UsageGroup {
- ADMIN, COMPACTION, CORE, PROCESS, OTHER
- }
Review Comment:
In 151a7da I reverted all changes to UsageGroup and then removed all uses of it except for the default method on KeywordExecutable. I changed UsageGroups and the new UsageGroup class to CommandGroups/CommandGroup. The new CommandGroup method and values are used in Main for printing and executing the commands.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#discussion_r2790599434
##########
start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java:
##########
@@ -44,10 +44,6 @@
*/
public interface KeywordExecutable {
- enum UsageGroup {
- ADMIN, COMPACTION, CORE, PROCESS, OTHER
- }
Review Comment:
I'm not sure I see an issue here. We have made breaking SPI changes before, even in minor releases IIRC.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#discussion_r2790316811
##########
shell/src/main/java/org/apache/accumulo/shell/ShellOptions.java:
##########
Review Comment:
It's confusing to combine the file that represents commons-cli options with JCommander options. It might be better to keep these separate. If they are entangled, it may make it difficult to modify CLI options for the shell command without breaking commands inside the shell, or vice versa.
##########
shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java:
##########
@@ -96,14 +95,15 @@ public String[] args(String... args) {
}
@Test
- public void testHelp() throws IOException {
- assertFalse(shell.config(args("--help")));
+ public void testHelp() throws Exception {
+ shell.execute(args("--help"));
assertTrue(output.get().startsWith("Usage"), "Did not print usage");
}
@Test
- public void testBadArg() throws IOException {
- assertFalse(shell.config(args("--bogus")));
+ public void testBadArg() throws Exception {
+ shell.execute(args("--bogus"));
+ assertTrue(shell.getExitCode() != 0);
Review Comment:
I think `assertNotEquals` is a thing, and it should have a more informative error message when it fails.
```suggestion
assertNotEquals(0, shell.getExitCode());
```
##########
core/src/main/java/org/apache/accumulo/core/cli/ClientKeywordExecutable.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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 ClientKeywordExecutable<O extends ClientOpts> implements KeywordExecutable {
+
+ private final O options;
Review Comment:
This looks like "zero options" 😺
Although it tends to be a convention, it is not strictly necessary to use a single character generic parameter type name. It could be "OPTS", or even "OPTIONS" instead. It is only used in a few places in this file, so it doesn't need to be excessively short.
##########
start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java:
##########
@@ -44,10 +44,6 @@
*/
public interface KeywordExecutable {
- enum UsageGroup {
- ADMIN, COMPACTION, CORE, PROCESS, OTHER
- }
Review Comment:
Moving this breaks the SPI. This should stay in place, so as not to break existing KeywordExecutable classes that users might have written. The use of a default method below was an attempt to ensure backward-compatibility, but that effort is undermined by breaking it this way.
Instead, a new type (maybe "CommandGroup"?) should be added, with a default method of its own. We can deprecate the usageGroup method and the UsageGroup enum, if it is no longer needed.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Command Groups, ClientKeywordExecutable [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #6121:
URL: https://github.com/apache/accumulo/pull/6121
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#discussion_r2790642298
##########
shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java:
##########
@@ -96,14 +95,15 @@ public String[] args(String... args) {
}
@Test
- public void testHelp() throws IOException {
- assertFalse(shell.config(args("--help")));
+ public void testHelp() throws Exception {
+ shell.execute(args("--help"));
assertTrue(output.get().startsWith("Usage"), "Did not print usage");
}
@Test
- public void testBadArg() throws IOException {
- assertFalse(shell.config(args("--bogus")));
+ public void testBadArg() throws Exception {
+ shell.execute(args("--bogus"));
+ assertTrue(shell.getExitCode() != 0);
Review Comment:
Changed in a1bcea8
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#discussion_r2790626824
##########
shell/src/main/java/org/apache/accumulo/shell/Shell.java:
##########
@@ -510,23 +481,20 @@ public String keyword() {
@Override
public UsageGroup usageGroup() {
- return UsageGroup.CORE;
+ return UsageGroups.CLIENT;
}
@Override
public String description() {
return "Runs Accumulo shell";
}
- @SuppressFBWarnings(value = "DM_EXIT", justification = "System.exit() from a main class is okay")
@Override
- public void execute(final String[] args) throws IOException {
+ public void execute(JCommander cl, ShellOptions options) throws Exception {
try {
- if (!config(args)) {
- System.exit(getExitCode());
+ if (config(cl, options)) {
+ start();
}
-
- System.exit(start());
} finally {
shutdown();
}
Review Comment:
One thing to note here is that the call to System.exit is removed from the Shell.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#issuecomment-3880802624
@dlmarion wrote:
> @ctubbsii - with this change the LocalCachingClassloader can declare it's own UsageGroup for its KeywordExecutables for 4.0. For example, if the UsageGroup is `ccl`, then the commands could be `./accumulo ccl init` and `./accumulo ccl create-manifest`.
Given that the classloader is intended to work across 2.1 and 4.0, it might be hard to make use of the changes here. Maybe it might make sense to put some of this KeywordExecutable+JCommander stuff in a separate library, that both 2.1 and 4.0 could leverage, though 2.1 might leverage it in a different way, so it doesn't break all the existing 2.1 commands.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#issuecomment-3891821352
> @dlmarion wrote:
>
> > @ctubbsii - with this change the LocalCachingClassloader can declare it's own UsageGroup for its KeywordExecutables for 4.0. For example, if the UsageGroup is `ccl`, then the commands could be `./accumulo ccl init` and `./accumulo ccl create-manifest`.
>
> Given that the classloader is intended to work across 2.1 and 4.0, it might be hard to make use of the changes here. Maybe it might make sense to put some of this KeywordExecutable+JCommander stuff in a separate library, that both 2.1 and 4.0 could leverage, though 2.1 might leverage it in a different way, so it doesn't break all the existing 2.1 commands.
Moving this into a separate library is a nice long-term goal and it might also make sense to add the KeywordExecutable interface to public API at that time.
However, that seems like a major task for minimal immediate gain. I can only think of the upgrade utility potentially using this common cli library due to it's need for cross-version support, but it's currently coupled with the accumulo source code.
The simplest solution would be adding a 2.1 branch to accumulo-classloaders. This would allow the cli utilities to work while following the same structure as the accumulo-testing repo.
I understand that the caching classloader is currently designed to work across multiple accumulo versions but unless we state otherwise, the repo will most likely diverge to support various accumulo versions in the future.
If that is a non-starter then an alternate solution would be decoupling the cli utility part of the classloader into separate modules that require different accumulo versions. I'm not a huge fan of this solution as it seems more complicated than managing a separate branch, but I'm open to discussions.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#issuecomment-3880341566
@ctubbsii - with this change the LocalCachingClassloader can declare it's own UsageGroup for its KeywordExecutables for 4.0. For example, if the UsageGroup is `ccl`, then the commands could be `./accumulo ccl init` and `./accumulo ccl create-manifest`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#issuecomment-3892269428
FYI that with the CommandGroup that was added in https://github.com/apache/accumulo/commit/151a7dad03df0930df3eaa867d3aadd59e0ab680 to restore the backward compatibility of KeywordExecutable, the classloader commands would continue to use their existing keyword() values in version 2.1 (`accumulo create-classloader-manifest` and `accumulo init-classloader-cache-dir`).
Currently in this PR there is no default CommandGroup on the KeywordExecutable interface. This makes it easier to track that all implementations have a CommandGroup specified until we have them where we want them.
If we made the default `OTHER`, then in 4.0 the classloader commands would be:
```
accumulo other create-classloader-manifest
accumulo other init-classloader-cache-dir
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#issuecomment-3892183216
Branching does seems like a good solution if not branching is going to lead to lots of complexity and maybe not an optimal experience when using the command line in 2.1 and 4.0. With branching can optimize the command line for what is available in each accumulo version. If we did branch could follow the same strategy as accumulo repo where bug fixes are merged to 2.1 then main. Could also update this projects readme w/ recommended versions eventually after accumulo releases 4.0. For now the readme in main (which would be against 4.0 code) could recommend using the 2.1 branch and 2.1 targeted release. Also branching this project avoids having to lock anything in prematurely, still gives ability to coevolve this main branch and accumulo main branch as new things are learned.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#discussion_r2790643289
##########
shell/src/main/java/org/apache/accumulo/shell/ShellOptions.java:
##########
Review Comment:
Reverted my merge of these classes in a1bcea8.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added Dynamic Usage Groups, Client Usage Group [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#discussion_r2790644343
##########
core/src/main/java/org/apache/accumulo/core/cli/ClientKeywordExecutable.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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 ClientKeywordExecutable<O extends ClientOpts> implements KeywordExecutable {
+
+ private final O options;
Review Comment:
Fixed ClientKeywordExecutable and ServerKeywordExecutable in a1bcea8.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org