Add parameter to make sure that log of updating IndexSetting be more detailed #49969
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
kkewwei
changed the title
DaveCTurner
requested changes
Dec 9, 2019
There was a problem hiding this comment.
Thanks @kkewwei, I think a better approach would be to adjust the logger that this AbstractScopedSettings uses. See e.g.
for the pattern we usually use to create a per-index logger.
You will also need to supply a test that demonstrates that the messages logged now include the index name.
|
Pinging @elastic/es-core-infra (:Core/Infra/Settings) |
added 2 commits
December 10, 2019 15:25
|
@DaveCTurner, your suggestion is very helpful. TBR |
DaveCTurner
requested changes
Dec 10, 2019
| final Setting.Property scope) { | ||
| final Setting.Property scope, | ||
| final String prefix) { | ||
| this.logger = Loggers.getLogger(this.getClass(), prefix); |
There was a problem hiding this comment.
I think it'd be better to allow subclasses to provide their own logger rather than just a String, because I don't think we should be changing the behaviour for the logging of cluster-level settings.
DaveCTurner
requested changes
Dec 10, 2019
There was a problem hiding this comment.
Sorry @kkewwei, I still think this is not quite the right approach. I don't think applySettings should take a Logger, I think we should be using the AbstractScopedSettings#logger field. Something like the following.
diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
index 81b3b92844d..54a272e0715 100644
--- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
+++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
@@ -57,3 +57,3 @@ public abstract class AbstractScopedSettings {
- protected final Logger logger = LogManager.getLogger(this.getClass());
+ private final Logger logger;
@@ -72,2 +72,3 @@ public abstract class AbstractScopedSettings {
final Setting.Property scope) {
+ this.logger = LogManager.getLogger(getClass());
this.settings = settings;
@@ -112,3 +113,4 @@ public abstract class AbstractScopedSettings {
- protected AbstractScopedSettings(Settings nodeSettings, Settings scopeSettings, AbstractScopedSettings other) {
+ protected AbstractScopedSettings(Settings nodeSettings, Settings scopeSettings, AbstractScopedSettings other, Logger logger) {
+ this.logger = logger;
this.settings = nodeSettings;
diff --git a/server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java
index f83012f2db3..29e78f53fc6 100644
--- a/server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java
+++ b/server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java
@@ -20,2 +20,3 @@ package org.elasticsearch.common.settings;
+import org.apache.logging.log4j.LogManager;
import org.elasticsearch.cluster.metadata.IndexMetaData;
@@ -26,2 +27,3 @@ import org.elasticsearch.cluster.routing.allocation.decider.MaxRetryAllocationDe
import org.elasticsearch.cluster.routing.allocation.decider.ShardsLimitAllocationDecider;
+import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.settings.Setting.Property;
@@ -188,3 +190,3 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
private IndexScopedSettings(Settings settings, IndexScopedSettings other, IndexMetaData metaData) {
- super(settings, metaData.getSettings(), other);
+ super(settings, metaData.getSettings(), other, Loggers.getLogger(IndexScopedSettings.class, metaData.getIndex()));
}
|
Thank you very much for sharing code, It‘ simpler and more reasonable. |
DaveCTurner
reviewed
Dec 17, 2019
server/src/test/java/org/elasticsearch/common/settings/SettingTests.java
Outdated
Show resolved
Hide resolved
|
It's very kind of your suggestion. |
DaveCTurner
requested changes
Jan 2, 2020
There was a problem hiding this comment.
Thanks @kkewwei, one final comment about a now-unused class.
| @@ -49,6 +62,27 @@ | |||
|
|
|||
| public class SettingTests extends ESTestCase { | |||
|
|
|||
| public static class MockAppender extends AbstractAppender { | |||
There was a problem hiding this comment.
This class is now unused
|
@elasticmachine test this please |
|
@elasticmachine update branch |
|
@elasticmachine test this please |
DaveCTurner
pushed a commit
that referenced
this pull request
Jan 3, 2020
Today we log changes to index settings like this:
updating [index.setting.blah] from [A] to [B]
The identity of the index whose settings were updated is conspicuously absent
from this message. This commit addresses this by adding the index name to these
messages.
Fixes #49818.
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
Today we log changes to index settings like this:
updating [index.setting.blah] from [A] to [B]
The identity of the index whose settings were updated is conspicuously absent
from this message. This commit addresses this by adding the index name to these
messages.
Fixes elastic#49818.
This was referenced Feb 3, 2020
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.
Relates #49818
Add the parameter: target, meaning whose setting will be changed