gemmellr commented on code in PR #5127:
URL: https://github.com/apache/activemq-artemis/pull/5127#discussion_r1705144195
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Create.java:
##########
@@ -77,6 +77,7 @@ public class Create extends InstallAbstract {
public static final String BIN_ARTEMIS_SERVICE = "bin/" + ARTEMIS_SERVICE;
public static final String ETC_ARTEMIS_PROFILE = "artemis.profile";
public static final String ETC_LOG4J2_PROPERTIES = "log4j2.properties";
+ public static final String ETC_LOG4J2_DEFAULT_PROPERTIES =
"log4j2-default.properties";
Review Comment:
log4j2-default.properties as a name seems likely to cause confusion given
that it will only be used when specified by the script for non-run commands,
and that the existing log4j2.properties will be used 'by default' by log4j in
any other cases. A different name for this, to more specifically reflect it is
explicitly used for 'non-run' tooling commands seems like it would be better.
##########
artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/artemis.profile:
##########
@@ -60,4 +60,6 @@ if [ "$1" = "run" ]; then :
# Uncomment for async profiler
# DEBUG_ARGS="-XX:+UnlockDiagnosticVMOptions -XX:+DebugNonSafepoints"
+else
+ JAVA_ARGS="$JAVA_ARGS
-Dlog4j2.configurationFile=${ARTEMIS_INSTANCE_ETC_URI}log4j2-default.properties
"
Review Comment:
This will override any existing -Dlog4j2.configurationFile config that
someone already has, or later tries to, set it via JAVA_ARGS. I know there is
a JAVA_ARGS_APPEND variable these days too, in order to allow for overriding
stuff at the end, but its not clear to me that you shouldnt still be able to
have set this in JAVA_ARGS given you could before now (allowing stuff like that
is one of the reasons the existing bits use log4j2.properties to be picked up
automatically, unless overridden by being specified).
What about adding this to the start of the args....or even setting this in
its own variable, used before JAVA_ARGS, then allowing people to change that.
It could be populated initially based on what is being executed, much like
this. People could then either set that variable, or continue to add
-Dlog4j2.configurationFile config in their JAVA_ARGS (or JAVA_ARGS_APPEND) to
override it.
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/Shell.java:
##########
@@ -122,7 +125,22 @@ public static void runShell(boolean printBanner) {
systemRegistry.setCommandRegistries(new
PicocliCommands(Artemis.buildCommand(isInstance, !isInstance, false)));
systemRegistry.cleanUp();
line = reader.readLine(prompt, rightPrompt,
(MaskingCallback) null, null);
- systemRegistry.execute(line);
+
+ if (isInstance && line.startsWith("run")) {
+ List<String> command = new ArrayList<>();
+ boolean IS_WINDOWS =
System.getProperty("os.name").toLowerCase().trim().startsWith("win");
+ if (IS_WINDOWS) {
+ command.add("cmd");
+ command.add("/c");
+ command.add(Paths.get(artemisInstance,
"artemis.cmd").toAbsolutePath().toString());
+ } else {
+ command.add(Paths.get(artemisInstance, "bin",
"artemis").toAbsolutePath().toString());
+ }
+ command.addAll(parser.parse(line, 0).words());
+ new ProcessBuilder(command).inheritIO().start().waitFor();
Review Comment:
I see that #5130 has actually instead disabled calling _run_ after calling
_shell_. I think thats a better way to go. Spawning [silently] from the shell
cmd seems likely to cause confusion with orphaned processes at some point.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
