vy commented on code in PR #2454:
URL: https://github.com/apache/logging-log4j2/pull/2454#discussion_r1561569850
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertySource.java:
##########
@@ -101,10 +101,12 @@ class Comparator implements
java.util.Comparator<PropertySource>, Serializable {
private static final long serialVersionUID = 1L;
@Override
- public int compare(final PropertySource o1, final PropertySource o2) {
- return Integer.compare(
- Objects.requireNonNull(o1).getPriority(),
- Objects.requireNonNull(o2).getPriority());
+ public int compare(final PropertySource left, final PropertySource
right) {
+ Objects.requireNonNull(left);
+ Objects.requireNonNull(right);
+ final int result = Integer.compare(left.getPriority(),
right.getPriority());
+ // Two property sources can have the same priority
+ return result != 0 || left.equals(right) ? result :
left.hashCode() - right.hashCode();
Review Comment:
I agree with Phillip Webb's proposal in
[LOG4J2-3618](https://issues.apache.org/jira/browse/LOG4J2-3618):
> use `equals/hashCode` to determine if items can be added, and only use the
Comparator for sorting
I am not able to see how this achieves that. I would be in favor of the
following instead:
1. Leave this comparator as is (only compare using ranks)
2. Initialize the `PropertiesUtil.Environment#sources` using `new
ConcurrentSkipListSet()`, i.e., no explicit comparator
3. Find iterations over `PropertiesUtil.Environment#sources`, and employ the
comparator there (i.e., `.stream().sorted(Comparator.INSTANCE).forEach(...)`)
_iff_ an order is needed.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -540,18 +552,18 @@ private synchronized void reload() {
final List<CharSequence> tokens =
PropertySource.Util.tokenize(key);
final boolean hasTokens = !tokens.isEmpty();
sources.forEach(source -> {
Review Comment:
Why do we only guard against the particular failure cases reported rather
than the entire block? That is, I was expecting:
```
keys.stream().filter(Objects.nonNull).forEach(key -> {
try {
// Old logic goes here verbatim
} catch (final Exception error) {
// Report the failure
}
});
for (final PropertySource source : sources) {
try {
// Old logic goes here verbatim
} catch (final Exception error) {
// Report the failure
}
}
```
--
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-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
