You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "JWT007 (via GitHub)" <gi...@apache.org> on 2024/11/04 23:06:06 UTC
[I] DefaultMergeStrategy - not accounting for AsyncWaitStrategyFactory when merging configurations (logging-log4j2)
JWT007 opened a new issue, #3172:
URL: https://github.com/apache/logging-log4j2/issues/3172
DefaultMergeStragegy (Log4j 2.24.1)
I *think* I may have encountered a potential error if using a CompositeConfiguration.
In the `DefaultMergeStrategy#mergeConfigurations`, it does a double loop over the source and target node's children - merging from the source into the target.
In these loops it provides special handling for PROPERTIES, SCRIPTS, APPENDERS, FILTERS and LOGGERS.
If the element tag name does not match one of these, the default behaviour is to add the children of the source element to the corresponding target element.
```
switch (toRootLowerCase(targetChildNode.getName())) {
case PROPERTIES:
case SCRIPTS:
case APPENDERS: { ... }
case LOGGERS: { ... }
default: {
targetChildNode.getChildren().addAll(sourceChildNode.getChildren());
isMerged = true;
break;
}
}
```
If however, two configurations had an `AsyncWaitStrategyFactory` element:
```
<Configuration>
<AsyncWaitStrategyFactory class="a.b.c.Foo">
</Configuration>
<Configuration>
<AsyncWaitStrategyFactory class="d.e.f.Bar">
</Configuration>
```
This element has no children, and the attributes are not merged.
The 'class' attribute after merging would be taken from the first occurrence `a.b.c.Foo` and not from the *override* 'd.e.f.Bar`.
I'll admit I have not tested this scenario, but reviewing the code it *logically* seems like it might cause a problem
NOTE: "Properties", "Scripts", "Appenders", "Filter" and "Loggers" are all collection elements and have no attributes - so it makes sense that for these no attributes are merged, but the default behavior should IMHO handle the possiblity of attributes and perform a merge of the element's attributes similar to that of the `DefaultMergeStrategy#mergeRootProperties` method.
--
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: notifications-unsubscribe@logging.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] DefaultMergeStrategy - not accounting for AsyncWaitStrategyFactory when merging configurations (logging-log4j2)
Posted by "JWT007 (via GitHub)" <gi...@apache.org>.
JWT007 commented on issue #3172:
URL: https://github.com/apache/logging-log4j2/issues/3172#issuecomment-2466244099
@ppkarwasz I am not sure why the bot didn't mark this as "waiting for maintainer" - not sure if it pops up for you?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] DefaultMergeStrategy - not accounting for AsyncWaitStrategyFactory when merging configurations (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on issue #3172:
URL: https://github.com/apache/logging-log4j2/issues/3172#issuecomment-2469950815
> what about `CustomLogLevels`? I think that os currently ignored too
Nice catch! :100:
That element should probably have a custom merging procedure.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] DefaultMergeStrategy - not accounting for AsyncWaitStrategyFactory when merging configurations (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on issue #3172:
URL: https://github.com/apache/logging-log4j2/issues/3172#issuecomment-2469764544
> I am not sure why the bot didn't mark this as "waiting for maintainer" - not sure if it pops up for you?
The workflow is in [`labeler.yaml`](https://github.com/apache/logging-log4j2/blob/2.x/.github/workflows/labeler.yaml) and is based on the affiliation of the reporter with the organization/repository/issue. I am not sure how GitHub assigns them. IIRC I had to add `CONTRIBUTOR` to the list, since some maintainers didn't show up as `MEMBER`. If you know a better way to check if the issue author has write permission to the repo, feel free to submit a PR.
> If however, two configurations had an `AsyncWaitStrategyFactory` element:
Personally I think that the `AsyncWaitStrategyFactory` needlessly complicates the configuration. There is a [`log4j2.asyncLoggerConfigWaitStrategy`](https://logging.apache.org/log4j/2.x/manual/systemproperties.html#log4j2.asyncLoggerConfigWaitStrategy) Log4j configuration property with a similar purpose.
Anyway, except `<Properties>`, `<Appenders>`, `<Loggers>`, `<Scripts>` and filters, all the other **direct** children of `<Configuration>` should replace the children of the same type IMHO.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] DefaultMergeStrategy - not accounting for AsyncWaitStrategyFactory when merging configurations (logging-log4j2)
Posted by "JWT007 (via GitHub)" <gi...@apache.org>.
JWT007 commented on issue #3172:
URL: https://github.com/apache/logging-log4j2/issues/3172#issuecomment-2469810762
what about `CustomLogLevels`? I think that os currently ignored too
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org