[WFCORE-4867] Highlight the config name in start message #4747
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
deps-ok
khermano
force-pushed
the
WFCORE-4867_from_main
branch
from
5344eb2
to
c471afe
Compare
rsvoboda
reviewed
Oct 5, 2021
There was a problem hiding this comment.
Having info about domain config after host config was a bit unexpected, but maybe it's the way it should be. Or it was process controller which was started first 🤔, not sure now.
[Host Controller] 11:30:11,026 INFO [org.jboss.as] (MSC service thread 1-2) WFLYSRV0049: WildFly Core 17.0.0.Beta8-SNAPSHOT - host config: host.xml - starting
[Host Controller] 11:30:11,511 INFO [org.wildfly.security] (Host Controller Service Threads - 4) ELY00001: WildFly Elytron version 1.17.0.Final
[Host Controller] 11:30:11,674 INFO [org.jboss.as.config] (Controller Boot Thread) WFLYSRV0049: - domain config: domain.xml - starting
Overall no concerns around this PR. @yersan can you pls review this PR as you were looking into the initial PR #4719.
yersan
requested changes
Oct 6, 2021
There was a problem hiding this comment.
I think this still needs some changes, at least for the localization.
It would be great also to have clear first what we want to be printed and when.
Do we really want this information at the beginning and repeat it at the end?
Why not use only the latest information printed when the server or host controller has started (successfully or with an error). For example, for standalone:
WFLYSRV0025: WildFly Full 26.0.0.Beta1-SNAPSHOT (WildFly Core 18.0.0.Beta1-SNAPSHOT) - started in 1977ms - Started 298 of 538 services (337 services are lazy, passive or on-demand) - Server configuration file in use: standalone.xml
For a Host controller (which is also a DC):
INFO [org.jboss.as] (Controller Boot Thread) WFLYSRV0025: WildFly Full 26.0.0.Beta1-SNAPSHOT (WildFly Core 18.0.0.Beta1-SNAPSHOT) - (Host Controller) started in 5920ms - Started 71 of 72 services (14 services are lazy, passive or on-demand) - Host Controller configuration files in use: domain.xml, host-master.xml
For a Host controller:
INFO [org.jboss.as] (Controller Boot Thread) WFLYSRV0025: WildFly Full 26.0.0.Beta1-SNAPSHOT (WildFly Core 18.0.0.Beta1-SNAPSHOT) - (Host Controller) started in 5920ms - Started 71 of 72 services (14 services are lazy, passive or on-demand) - Host Controller configuration files in use: host-slave.xml
Other ideas? Thoughts?
@khermano I think the above could be achieved around these lines on ServerService and DomainModelControllerService adding more information to the bootstrapListener.printBootStatistics() call, or when there is a failure, by using the ROOT_LOGGER.unsuccessfulBoot().
| @@ -119,6 +120,10 @@ public void initializeDomainConfigurationPersister(boolean slave) { | |||
| // Store this back to environment so mgmt api that exposes it can still work | |||
| environment.setDomainConfigurationFile(domainConfigurationFile); | |||
|
|
|||
| if (environment.getDomainConfigurationFile() != null) { | |||
| ServerLogger.CONFIG_LOGGER.serverStarting("- domain config: " + environment.getDomainConfigurationFile().getMainFile().getName() + " -"); | |||
| } | |||
There was a problem hiding this comment.
In my opinion, printing this kind of message is not under the responsibility of HostControllerConfigurationPersister.java, so this doesn't look like the correct place for it. In addition, at this point, the HC has started with an empty domain config, and describing that the Domain config is starting doesn't look correct to me.
In any case, we have to handle these messages via the loggers so they can be localized later, we should not hardcode messages that are going to get printed on the console outside of the loggers.
| @@ -120,7 +120,7 @@ public void start(StartContext context) throws StartException { | |||
| //Moved to AbstractControllerService.start() | |||
| // processState.setStarting(); | |||
| final ProductConfig config = environment.getProductConfig(); | |||
| final String prettyVersion = config.getPrettyVersionString(); | |||
| final String prettyVersion = config.getPrettyVersionString() + " - host config: " + environment.getHostConfigurationFile().getMainFile().getName() + " -"; | |||
There was a problem hiding this comment.
The same issue as described before regarding the localization. This is also modifying the prettyVersion which is printed at the end of the boot process, so we are duplicating the information. I'm not saying that's wrong, but for asking whether we want this information at the beginning and at the end. I'm not sure.
Yes, @rsvoboda it is a bit odd. It is not the process controller, the reason is the Host Controller starts first with an empty domain configuration, and later when the domain configuration is determined it prints the |
|
Hi, thank you both for your comments and sorry for the delay, I was on PTO. |
khermano
force-pushed
the
WFCORE-4867_from_main
branch
from
c471afe
to
7e8c873
Compare
|
Core - Full Integration Build 11059 outcome was FAILURE using a merge of 7e8c873 Failed tests |
khermano
force-pushed
the
WFCORE-4867_from_main
branch
from
7e8c873
to
f2130c2
Compare
yersan
requested changes
Oct 13, 2021
There was a problem hiding this comment.
@jmesnil / @bstansberry It would be great if you can add your opinion on the message format since we plan to modify a well known message adding more information; e.g:
[org.jboss.as] (Controller Boot Thread) WFLYSRV0025: WildFly Full 26.0.0.Beta1-SNAPSHOT (WildFly Core 18.0.0.Beta2-SNAPSHOT) (Host Controller) started in 2325ms - Started 71 of 71 services (13 services are lazy, passive or on-demand) - Host Controller configuration files in use: domain.xml, host-master.xml
can be considered too long, my only doubt is whether we should split it into two. The motivation is there will be a feature incoming that will use an alias for the configuration files, so it would be nice to see on the default console the name of the file in use.
| @@ -104,10 +104,18 @@ private void done(final long bootstrapTime, final StabilityStatistics statistics | |||
| final int problem = statistics.getProblemsCount(); | |||
| final int started = statistics.getStartedCount(); | |||
| if (failed == 0 && problem == 0) { | |||
| ServerLogger.AS_ROOT_LOGGER.startedClean(prettyVersion, bootstrapTime, started, active + passive + onDemand + never + lazy, onDemand + passive + lazy); | |||
| if (isHostController) { | |||
| ServerLogger.AS_ROOT_LOGGER.startedClean(prettyVersion, bootstrapTime, started, active + passive + onDemand + never + lazy, onDemand + passive + lazy, "Host Controller configuration files in use: " + configFiles); | |||
There was a problem hiding this comment.
"Host Controller configuration files in use: "
This text must be in the ServerLogger itself. The point is the ServerLogger texts are translated to different languages, we have to avoid to hardcode texts outside of these loggers if that text is going to be printed by default to the standard output.
There was a problem hiding this comment.
Thank you for this info :) So I will create 6 new messages in total - 3 for startedClean (HC master, HC slave and standalone configuration) and 3 for startedWithErrors (same as before).
| @@ -897,7 +897,8 @@ public void execute(OperationContext context, ModelNode operation) throws Operat | |||
| Notification notification = new Notification(ModelDescriptionConstants.BOOT_COMPLETE_NOTIFICATION, PathAddress.pathAddress(PathElement.pathElement(CORE_SERVICE, MANAGEMENT), | |||
| PathElement.pathElement(SERVICE, MANAGEMENT_OPERATIONS)), ControllerLogger.MGMT_OP_LOGGER.bootComplete()); | |||
| getNotificationSupport().emit(notification); | |||
| bootstrapListener.printBootStatistics(); | |||
| bootstrapListener.printBootStatistics(true, environment.getDomainConfigurationFile().getMainFile().getName() | |||
| + ", " + environment.getHostConfigurationFile().getMainFile().getName()); | |||
There was a problem hiding this comment.
Wouldn't it be better to pass a variable argument and leave the format, in this case, separated by ", " to the logger itself?
I feel how the files will be printed is the responsibility of the logger message and not to the argument passed by to the printBootStatistics()
There was a problem hiding this comment.
Sure I can do that but it will mean that I have to create 6 messages instead of 4 as I meantioned before, because the message would take more paramaters which I would probably sent in list, I think it would be easier if I'll push my changes so you could see what I mean.
| @@ -897,7 +897,8 @@ public void execute(OperationContext context, ModelNode operation) throws Operat | |||
| Notification notification = new Notification(ModelDescriptionConstants.BOOT_COMPLETE_NOTIFICATION, PathAddress.pathAddress(PathElement.pathElement(CORE_SERVICE, MANAGEMENT), | |||
| PathElement.pathElement(SERVICE, MANAGEMENT_OPERATIONS)), ControllerLogger.MGMT_OP_LOGGER.bootComplete()); | |||
| getNotificationSupport().emit(notification); | |||
| bootstrapListener.printBootStatistics(); | |||
| bootstrapListener.printBootStatistics(true, environment.getDomainConfigurationFile().getMainFile().getName() | |||
There was a problem hiding this comment.
environment.getDomainConfigurationFile() will be null if we are on an slave HC. You have to check this possibility.
| @@ -897,7 +897,8 @@ public void execute(OperationContext context, ModelNode operation) throws Operat | |||
| Notification notification = new Notification(ModelDescriptionConstants.BOOT_COMPLETE_NOTIFICATION, PathAddress.pathAddress(PathElement.pathElement(CORE_SERVICE, MANAGEMENT), | |||
| PathElement.pathElement(SERVICE, MANAGEMENT_OPERATIONS)), ControllerLogger.MGMT_OP_LOGGER.bootComplete()); | |||
| getNotificationSupport().emit(notification); | |||
| bootstrapListener.printBootStatistics(); | |||
| bootstrapListener.printBootStatistics(true, environment.getDomainConfigurationFile().getMainFile().getName() | |||
| + ", " + environment.getHostConfigurationFile().getMainFile().getName()); | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
What about if there is a problem? It can be considered optional, but It would be nice to get the name of the configuration file in use if your server fails to start. Keep in mind if an HC fails to start before processing the domain configuration file, you will end here with a null environment.getDomainConfigurationFile()
There was a problem hiding this comment.
Do you mean the else brach starting with "//Die!" comment? I will add some new messages in new push to see if that is what you mean?
There was a problem hiding this comment.
Yes, correct, I think it would be useful if possible and your server / host doesn't start to know which configuration file is in use, so you as a user could have more possibilities to identify the issue. That's optional, feel free to add the possibility if you think it is useful and don't make it too much complex.
|
Core - Full Integration Build 11062 outcome was FAILURE using a merge of f2130c2 Failed tests |
khermano
force-pushed
the
WFCORE-4867_from_main
branch
from
f2130c2
to
925fbfe
Compare
|
Core - Full Integration Build 11000 outcome was FAILURE using a merge of 925fbfe Failed tests |
khermano
force-pushed
the
WFCORE-4867_from_main
branch
from
925fbfe
to
5cf4f75
Compare
|
Core - Full Integration Build 11002 outcome was FAILURE using a merge of 5cf4f75 Failed tests |
yersan
requested changes
Oct 26, 2021
| } | ||
| } else { | ||
| // Die! | ||
| String failed = ROOT_LOGGER.unsuccessfulBoot(); | ||
| String failed; | ||
| if (environment != null & environment.getHostConfigurationFile() != null & environment.getDomainConfigurationFile() != null) { |
There was a problem hiding this comment.
environment and environment.getHostConfigurationFile() never will be null here.
Comment on lines
234
to
240
| /*@LogMessage(level = INFO) | ||
| @Message(id = 25, value = "%s started in %dms - Started %d of %d services (%d services are lazy, passive or on-demand) - %s") | ||
| void startedClean(String prettyVersionString, long time, int startedServices, int allServices, int passiveOnDemandServices, String configFilesString); | ||
|
|
||
| @LogMessage(level = ERROR) | ||
| @Message(id = 26, value = "%s started (with errors) in %dms - Started %d of %d services (%d services failed or missing dependencies, %d services are lazy, passive or on-demand)") | ||
| void startedWitErrors(String prettyVersionString, long time, int startedServices, int allServices, int problemServices, int passiveOnDemandServices); | ||
| @Message(id = 26, value = "%s started (with errors) in %dms - Started %d of %d services (%d services failed or missing dependencies, %d services are lazy, passive or on-demand) - %s") | ||
| void startedWitErrors(String prettyVersionString, long time, int startedServices, int allServices, int problemServices, int passiveOnDemandServices, String configFilesString);*/ |
There was a problem hiding this comment.
Sadly, we cannot change the Ids of these two important messages:
- They are used considerably for tests on the cloud images
- We should not use different ids for messages that means the same, you have added several messages with slightly different text, but they are describing the same: The server or the host controller are started / failed.
You should compose the final messages appending dynamic parts of passing the dynamic parts as arguments to the message, but we cannot use different Ids and or change the current ones.
Those messages and the parts that compose them should be suitable for i8n, I think one possibility to compose them is to use messages without Ids as component parts for the final message, see
as an example
Comment on lines
427
to
429
| if (configuration == null || configuration.getServerEnvironment() == null) { | ||
| throw new IllegalStateException(); | ||
| } |
There was a problem hiding this comment.
confguration and configuration.getServerEnvironment() are never null on ServerService
Comment on lines
900
to
902
| if (environment == null || environment.getHostConfigurationFile() == null) { | ||
| throw new IllegalStateException(); | ||
| } |
There was a problem hiding this comment.
environment and environment.getHostConfigurationFile() are never null for DomainModelControllerService.boot()
| @@ -897,7 +897,8 @@ public void execute(OperationContext context, ModelNode operation) throws Operat | |||
| Notification notification = new Notification(ModelDescriptionConstants.BOOT_COMPLETE_NOTIFICATION, PathAddress.pathAddress(PathElement.pathElement(CORE_SERVICE, MANAGEMENT), | |||
| PathElement.pathElement(SERVICE, MANAGEMENT_OPERATIONS)), ControllerLogger.MGMT_OP_LOGGER.bootComplete()); | |||
| getNotificationSupport().emit(notification); | |||
| bootstrapListener.printBootStatistics(); | |||
| bootstrapListener.printBootStatistics(true, environment.getDomainConfigurationFile().getMainFile().getName() | |||
| + ", " + environment.getHostConfigurationFile().getMainFile().getName()); | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
Yes, correct, I think it would be useful if possible and your server / host doesn't start to know which configuration file is in use, so you as a user could have more possibilities to identify the issue. That's optional, feel free to add the possibility if you think it is useful and don't make it too much complex.
khermano
force-pushed
the
WFCORE-4867_from_main
branch
from
5cf4f75
to
25110f1
Compare
|
@yersan I added some changes earlier but I am not sure if you were notified about that so I rather letting you know. If you could look at that later when you have some spare time it would be great. Thank you and sorry for the delay. |
yersan
requested changes
Nov 22, 2021
Comment on lines
108
to
115
| String append = ""; | ||
| if (configFiles != null & !configFiles.isEmpty() & isHostController & isMaster & configFiles.size() == 2) { | ||
| append = ServerLogger.AS_ROOT_LOGGER.hostControllerMasterConfigFilesInUse(configFiles.get(0), configFiles.get(1)); | ||
| } else if (configFiles != null & !configFiles.isEmpty() & isHostController & !isMaster & configFiles.size() == 1) { | ||
| append = ServerLogger.AS_ROOT_LOGGER.hostControllerSlaveConfigFileInUse(configFiles.get(0)); | ||
| } else if (configFiles != null & !configFiles.isEmpty() & !isHostController & configFiles.size() == 1) { | ||
| append = ServerLogger.AS_ROOT_LOGGER.serverConfigFileInUse(configFiles.get(0)); | ||
| } |
There was a problem hiding this comment.
This piece of code is repeated in an if/else. You can extract it from that flow control structure to avoid redundancy.
In addition, && is preferred here since it will break the shortcut as soon as it detects on operand as false, instead of & that will evaluate all the operands aways.
There was a problem hiding this comment.
Removing the isMaster argument, you could arrive at something like the following:
String append = "";
if (configFiles != null && !configFiles.isEmpty()) {
if (isHostController) {
if (configFiles.size() == 2)
// master host contoller message
else {
// slave host controller message
}
} else {
// server message
}
}
That will facilitate the debugging and it is more redeable
| @@ -68,7 +69,7 @@ public StabilityMonitor getStabilityMonitor() { | |||
| return monitor; | |||
| } | |||
|
|
|||
| public void printBootStatistics() { | |||
| public void printBootStatistics(boolean isHostController, boolean isMaster, List<String> configFiles) { | |||
There was a problem hiding this comment.
Remove the boolean isMaster, you don't need it, you can infer the behavior by using configFiles.size() == 2, (configFiles.size() == 2, then you are in a master DC)
| @@ -424,10 +424,18 @@ public void undeploy(DeploymentUnit context) { | |||
| Notification notification = new Notification(ModelDescriptionConstants.BOOT_COMPLETE_NOTIFICATION, PathAddress.pathAddress(PathElement.pathElement(CORE_SERVICE, MANAGEMENT), | |||
| PathElement.pathElement(SERVICE, MANAGEMENT_OPERATIONS)), ServerLogger.AS_ROOT_LOGGER.bootComplete()); | |||
| getNotificationSupport().emit(notification); | |||
| bootstrapListener.printBootStatistics(); | |||
| List<String> configFiles = new ArrayList<String>(); | |||
There was a problem hiding this comment.
Minor detail, you can use Java Type inference capabilities, so instead of new ArrayList<String>() use new ArrayList<>() here.
| //////////////////////////////////////////////// | ||
| //Messages without IDs | ||
|
|
||
| @Message(id = Message.NONE, value = " Config files in use: %s, %s") |
There was a problem hiding this comment.
s/Config files/Host Controller configuration files
There was a problem hiding this comment.
Remove the extra white space at the beginning
| @Message(id = Message.NONE, value = " Config files in use: %s, %s") | ||
| String configFilesInUse(String domainConfigFile, String hostConfigFile); | ||
|
|
||
| @Message(id = Message.NONE, value = " Config file in use: %s") |
There was a problem hiding this comment.
s/Config file/Host Controller configuration file
There was a problem hiding this comment.
Remove the extra white space at the beginning
| @@ -897,11 +897,22 @@ public void execute(OperationContext context, ModelNode operation) throws Operat | |||
| Notification notification = new Notification(ModelDescriptionConstants.BOOT_COMPLETE_NOTIFICATION, PathAddress.pathAddress(PathElement.pathElement(CORE_SERVICE, MANAGEMENT), | |||
| PathElement.pathElement(SERVICE, MANAGEMENT_OPERATIONS)), ControllerLogger.MGMT_OP_LOGGER.bootComplete()); | |||
| getNotificationSupport().emit(notification); | |||
| bootstrapListener.printBootStatistics(); | |||
| List<String> configFiles = new ArrayList<String>(); | |||
There was a problem hiding this comment.
Minor detail, you can use Java Type inference capabilities, so instead of new ArrayList<String>() use new ArrayList<>() here.
khermano
force-pushed
the
WFCORE-4867_from_main
branch
4 times, most recently
from
8085a94
to
c5b44ef
Compare
khermano
force-pushed
the
WFCORE-4867_from_main
branch
from
c5b44ef
to
fb76f53
Compare
|
Core - Full Integration Build 11201 outcome was UNKNOWN using a merge of fb76f53 |
|
Core - Full Integration Build 11095 outcome was UNKNOWN using a merge of fb76f53 |
|
Core - Full Integration Build 11202 outcome was UNKNOWN using a merge of fb76f53 |
|
Core - Full Integration Build 11096 outcome was UNKNOWN using a merge of fb76f53 |
|
Core - Full Integration Build 11203 outcome was UNKNOWN using a merge of fb76f53 |
|
Core - Full Integration Build 11204 outcome was UNKNOWN using a merge of fb76f53 |
|
Core - Full Integration Build 11097 outcome was UNKNOWN using a merge of fb76f53 |
|
Core - Full Integration Build 11209 outcome was FAILURE using a merge of fb76f53 Failed tests |
|
Core - Full Integration Build 11212 outcome was FAILURE using a merge of fb76f53 |
yersan
approved these changes
Nov 29, 2021
There was a problem hiding this comment.
@jmesnil / @bstansberry Changes on this PR look good to me, but I would like to get your feedback before merging. With this PR we were going to get these logs:
Standalone:
09:10:00,539 INFO [org.jboss.as] (Controller Boot Thread) WFLYSRV0025: WildFly Core 18.0.0.Beta4-SNAPSHOT started in 1550ms - Started 80 of 82 services (16 services are lazy, passive or on-demand) - Server configuration file in use: standalone.xml
Domain Mode:
[Host Controller] 09:12:13,221 INFO [org.jboss.as] (Controller Boot Thread) WFLYSRV0025: WildFly Core 18.0.0.Beta4-SNAPSHOT (Host Controller) started in 4438ms - Started 70 of 72 services (15 services are lazy, passive or on-demand) - Host Controller configuration files in use: domain.xml, host.xml
jmesnil
approved these changes
Dec 3, 2021
There was a problem hiding this comment.
@yersan we are not losing any existing info and add an important one. I'm fine with that changes.
@bstansberry do you see any issue with adding info to this essential WFLYSRV0025 log?
jmesnil
added
the
ready-for-merge
|
@khermano thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: https://issues.redhat.com/browse/WFCORE-4867
related to PR #4719