You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "brusdev (via GitHub)" <gi...@apache.org> on 2024/08/05 06:29:42 UTC
[PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
brusdev opened a new pull request, #5127:
URL: https://github.com/apache/activemq-artemis/pull/5127
The run command uses log4j2.properties and all other commands use log4j2-default.properties.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #5127:
URL: https://github.com/apache/activemq-artemis/pull/5127#discussion_r1705276839
##########
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:
My point is that it feels like 'log4j2-default' isnt a very clear descriptive name for everyone else and could easily be misunderstood. I know that we will understand what its for, but it seems like something others could misinterpret, i.e those who havent seen the Jira/this discussion/this PR to understand exactly why that file is being created and what that file is not used for. Especially when in actual Log4J2 terms the existing log4j2.properties is actually 'the default file' (to Log4J it is the highest preendence non-test configuration file, and what will get used if you dont specify -Dlog4j2.configurationFile=foo). Something more descriptive of its usage like e.g log4j2-tools/utilities/foo.properties makes more explicit what the file is being aimed at than just a '-default' suffix.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #5127:
URL: https://github.com/apache/activemq-artemis/pull/5127#discussion_r1705216204
##########
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:
Good point, I'll fix it.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #5127:
URL: https://github.com/apache/activemq-artemis/pull/5127#discussion_r1708700568
##########
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:
Now I see your point, I'll use the `utility` suffix.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev closed pull request #5127: ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands
URL: https://github.com/apache/activemq-artemis/pull/5127
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #5127:
URL: https://github.com/apache/activemq-artemis/pull/5127#discussion_r1705207850
##########
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'll remove this part.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #5127:
URL: https://github.com/apache/activemq-artemis/pull/5127#discussion_r1705211573
##########
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:
I'm not sure to get your point. My point is that only the run command uses log4j2.properties all other commands use log4j2-default.properties.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4785 Add log4j2-default.properties for non-run CLI commands [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on PR #5127:
URL: https://github.com/apache/activemq-artemis/pull/5127#issuecomment-2275022045
Replaced by #5136
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact