[
https://issues.apache.org/jira/browse/ARTEMIS-4372?focusedWorklogId=873501&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-873501
]
ASF GitHub Bot logged work on ARTEMIS-4372:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 28/Jul/23 10:24
Start Date: 28/Jul/23 10:24
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #4565:
URL: https://github.com/apache/activemq-artemis/pull/4565#discussion_r1277341990
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/messages/ConnectionAbstract.java:
##########
@@ -172,6 +197,13 @@ private ConnectionFactory
createAMQPConnectionFactory(String brokerURL,
// if a connection exception will ask for the URL, user and password
getActionContext().err.println("Connection failed::" +
e.getMessage());
cf = new JmsConnectionFactory(inputUser(user),
inputPassword(password), inputBrokerURL(brokerURL));
+ try {
+ Connection connection = cf.createConnection();
+ connection.close();
+ connectionSucces(brokerURL, user, password);
Review Comment:
Use tryConnect() like done elsewhere?
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/messages/DestAbstract.java:
##########
@@ -21,29 +21,29 @@
import javax.jms.JMSException;
import javax.jms.Session;
-import com.github.rvesse.airline.annotations.Option;
import org.apache.activemq.artemis.cli.factory.serialize.MessageSerializer;
import org.apache.activemq.artemis.cli.factory.serialize.XMLMessageSerializer;
import org.apache.activemq.artemis.jms.client.ActiveMQDestination;
+import picocli.CommandLine.Option;
public class DestAbstract extends ConnectionAbstract {
- @Option(name = "--destination", description = "Destination to be used. It
can be prefixed with queue:// or topic:// and can be an FQQN in the form of
<address>::<queue>. Default: queue://TEST.")
+ @Option(names = "--destination", description = "Destination to be used. It
can be prefixed with queue:// or topic:// and can be an FQQN in the form of
<address>::<queue>. Default: queue://TEST.")
String destination = "queue://TEST";
- @Option(name = "--message-count", description = "Number of messages to act
on. Default: 1000.")
+ @Option(names = "--message-count", description = "Number of messages to act
on. Default: 1000.")
int messageCount = 1000;
- @Option(name = "--sleep", description = "Time wait between each message.")
+ @Option(names = "--sleep", description = "Time wait between each message.")
int sleep = 0;
- @Option(name = "--txt-size", description = "Transaction batch size.")
+ @Option(names = {"--txt-size", "--tx-batch-size"}, description =
"Transaction batch size. (--txt-size is deprecated)")
Review Comment:
The non-deprecated option name being first would read better (unless the cli
is reverse-ordering the options)
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/Artemis.java:
##########
@@ -194,62 +189,111 @@ public static Object execute(boolean inputEnabled,
boolean useSystemOut, File ar
* Useful on test cases
*/
private static Object internalExecute(File artemisHome, File
artemisInstance, File etcFolder, String[] args) throws Exception {
- return internalExecute(artemisHome, artemisInstance, etcFolder, args,
ActionContext.system());
+ return internalExecute(artemisHome, artemisInstance, etcFolder, args,
new ActionContext());
}
public static Object internalExecute(File artemisHome, File
artemisInstance, File etcFolder, String[] args, ActionContext context) throws
Exception {
- Action action = builder(artemisInstance).build().parse(args);
- action.setHomeValues(artemisHome, artemisInstance, etcFolder);
+ boolean isInstance = artemisInstance != null ||
System.getProperty("artemis.instance") != null;
+ CommandLine commandLine = buildCommand(isInstance, !isInstance);
+
+ Object userObject = parseAction(commandLine, args);
+ if (userObject instanceof Action) {
Review Comment:
Maybe add an Objects.requireNonNull() here...or otherwise cover potential
for NPE when throwing the new IAE below.
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/messages/ConnectionAbstract.java:
##########
@@ -180,28 +212,62 @@ private ConnectionFactory
createAMQPConnectionFactory(String brokerURL,
}
protected ActiveMQConnectionFactory createCoreConnectionFactory() {
+ recoverSuccessPassword();
return createCoreConnectionFactory(brokerURL, user, password, clientID);
}
+ private void recoverSuccessPassword() {
+ if (CONNECTION_SUCCESS.get() != null) {
+ ConnectionSuccess success = CONNECTION_SUCCESS.get();
+ if (this.user == null) {
+ this.user = success.user;
+ }
+ if (this.password == null) {
+ this.password = success.password;
+ }
+ if (this.brokerURL == null) {
+ this.brokerURL = success.uri;
+ }
+
+ }
+ }
+
+ void connectionSucces(String brokerURL, String user, String password) {
Review Comment:
Typo in method name, missing s at end...connectionSucceeded might be better.
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/ActionAbstract.java:
##########
@@ -205,6 +205,20 @@ the ARTEMIS_HOME variable will include back slashes (An
invalid file URI charact
return brokerHome;
}
+ @Override
+ public void run() {
+ try {
+ execute(getActionContext());
+ } catch (Throwable e) {
+ e.printStackTrace();
Review Comment:
Is this catering to the shell usage, which already catches+prints by itself?
Or regular CLI usage?
Actually..when is this run() implementation used exactly? The 'internal
execute' method is specifically checking for Action and handling it first...so
wont all of the stuff using this abstract class go through that path instead?
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/messages/ConnectionAbstract.java:
##########
@@ -159,11 +176,19 @@ private ConnectionFactory
createAMQPConnectionFactory(String brokerURL,
try {
Connection connection = cf.createConnection();
connection.close();
+ connectionSucces(brokerURL, user, password);
Review Comment:
Use tryConnect() like done elsewhere?
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/messages/ConnectionAbstract.java:
##########
@@ -180,28 +212,62 @@ private ConnectionFactory
createAMQPConnectionFactory(String brokerURL,
}
protected ActiveMQConnectionFactory createCoreConnectionFactory() {
+ recoverSuccessPassword();
return createCoreConnectionFactory(brokerURL, user, password, clientID);
}
+ private void recoverSuccessPassword() {
Review Comment:
Its doing more than the name suggests. A clearer name, maybe also around the
expected usage, would be better. Perhaps recoverSavedConnectionInfo? Similarly
renaming the class/fields below something related around 'connection info'.
Issue Time Tracking
-------------------
Worklog Id: (was: 873501)
Time Spent: 2h 40m (was: 2.5h)
> Move CLI framework to picocli and implement auto-complete
> ---------------------------------------------------------
>
> Key: ARTEMIS-4372
> URL: https://issues.apache.org/jira/browse/ARTEMIS-4372
> Project: ActiveMQ Artemis
> Issue Type: New Feature
> Reporter: Clebert Suconic
> Assignee: Clebert Suconic
> Priority: Major
> Fix For: 2.31.0
>
> Time Spent: 2h 40m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
