Added Dynamic Command Groups, ClientKeywordExecutable #6121
Conversation
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
|
@ctubbsii - with this change the LocalCachingClassloader can declare it's own UsageGroup for its KeywordExecutables for 4.0. For example, if the UsageGroup is |
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
I really like combining the option parsing with the KeywordExecutable, and also by using the subclasses, you can improve the boilerplate for all this without modifying the interface in an incompatible way.
But, changing the UsageGroup enum breaks SPI compatibility unnecessarily.
core/src/main/java/org/apache/accumulo/core/cli/ClientKeywordExecutable.java
Outdated
Show resolved
Hide resolved
shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java
Outdated
Show resolved
Hide resolved
| enum UsageGroup { | ||
| ADMIN, COMPACTION, CORE, PROCESS, OTHER | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this comment.
I'm not sure I see an issue here. We have made breaking SPI changes before, even in minor releases IIRC.
There was a problem hiding this 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.
|
@dlmarion wrote:
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. |
| 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(); | ||
| } |
There was a problem hiding this comment.
One thing to note here is that the call to System.exit is removed from the Shell.
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. |
|
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. |
|
FYI that with the CommandGroup that was added in 151a7da to restore the backward compatibility of KeywordExecutable, the classloader commands would continue to use their existing keyword() values in version 2.1 ( 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 |
dlmarion
changed the title
4 participants
This change does the following: