Skip to content

Fix test clusters to respect user-provided ordering of settings. #137179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mosche wants to merge 3 commits into from
Closed

Fix test clusters to respect user-provided ordering of settings. #137179

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bec7653
Fix test clusters to respect user-provided ordering of settings.
mosche Oct 27, 2025
ada4873
Revert "Fix test clusters to respect user-provided ordering of settin…
mosche Oct 28, 2025
528c1f3
Restore sorting when applying loggers
mosche Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions server/src/main/java/org/elasticsearch/common/logging/LogConfigurator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.apache.logging.log4j.util.Unbox;
import org.elasticsearch.cluster.ClusterName;
import org.elasticsearch.common.logging.internal.LoggerFactoryImpl;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.env.Environment;
Expand All @@ -55,6 +56,7 @@
import java.nio.file.attribute.BasicFileAttributes;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -352,6 +354,7 @@ private static void configureLoggerLevels(final Settings settings) {
Loggers.LOG_LEVEL_SETTING.getAllConcreteSettings(settings)
// do not set a log level for a logger named level (from the default log setting)
.filter(s -> s.getKey().equals(Loggers.LOG_DEFAULT_LEVEL_SETTING.getKey()) == false)
.sorted(Comparator.comparing(Setting::getKey))
Copy link
Contributor Author

@mosche mosche Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for demonstration, currently we'd have to do something like this. This is unfortunate, internally everything is already sorted, but that's lost when generating the keySet.

Something similar would need to be done in ClusterSettings.LoggingSettingUpdater. The code there however doesn't use concrete settings and follows a different strategy. Very easy to get inconsistent behavior that way :/
This could benefit a lot from a more through redesign

Copy link
Member

@rjernst rjernst Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of ordering these, could we keep track of whether a particular logger has an overridden level, or inherits it? Then regardless of which order we process updating, we can decide only to apply/descend into a logger if it is inheriting. Concretely imagine we are setting two log levels:

logger.foo.bar.baz: TRACE
logger.foo.bar: DEBUG

With the current code we would first set foo.bar.baz to trace, but then incorrectly override it to debug when processing the second logging setting. This happens because we blindly descend from foo.bar to set debug level. Ideally we could just look at the logger to see if it has an explicit level. Unfortunately we set the level at each part of the hierarchy, see #20463.

My thought is if we could keep track of something like the explicit level (but that knows about our descent when setting levels), we could skip by that part of the hierarchy, eg foo.bar.baz if we see an explicit level already set.

Copy link
Member

@rjernst rjernst Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another relevant past issue/discussion I found: #65208

Copy link
Contributor Author

@mosche mosche Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjernst Similar to what you suggested, I introduced a configuration context here #137319

In #65208 the issue seems to be a mismatch between cluster state and actual logger configurations in use. That sounds like a much larger and fairly complex issue.

.forEach(s -> {
final Level level = s.get(settings);
Loggers.setLevel(LogManager.getLogger(s.getKey().substring("logger.".length())), level);
Expand Down