Core: Drop settings member from AbstractComponent #35083

nik9000 · 2018-10-30T12:48:35Z

Drops the Settings member from AbstractComponent, moving it from the
base class on to the classes that use it. For the most part this is a
mechanical change that doesn't drop Settings accesses. The one
exception to this is naming threads where it switches from an invocation
that passes Settings and extracts the node name to one that explicitly
passes the node name.

This change doesn't drop the Settings argument from
AbstractComponent's ctor because this change is big enough as is.
We'll do that in a follow up change.

Drops the `Settings` member from `AbstractComponent`, moving it from the base class on to the classes that use it. For the most part this is a mechanical change that doesn't drop `Settings` accesses. The one exception to this is naming threads where it switches from an invocation that passes `Settings` and extracts the node name to one that explicitly passes the node name. This change doesn't drop the `Settings` argument from `AbstractComponent`'s ctor because this change is big enough as is. We'll do that in a follow up change.

elasticmachine · 2018-10-30T12:48:39Z

Pinging @elastic/es-core-infra

nik9000 · 2018-10-30T12:49:15Z

modules/repository-url/src/main/java/org/elasticsearch/repositories/url/URLRepository.java

@@ -82,21 +82,21 @@ public URLRepository(RepositoryMetaData metadata, Environment environment,
                         NamedXContentRegistry namedXContentRegistry) {
        super(metadata, environment.settings(), namedXContentRegistry);

-        if (URL_SETTING.exists(metadata.settings()) == false && REPOSITORIES_URL_SETTING.exists(settings) ==  false) {
+        if (URL_SETTING.exists(metadata.settings()) == false && REPOSITORIES_URL_SETTING.exists(environment.settings()) ==  false) {


All of these repositories use environment.settings() instead of the "normal" settings object. This seems a bit fishy to be honest but it is what they have always done.

Maybe this is a case for removing settings() from Environment separately? Tbh, I've been guilty of just passing in the environment and using the settings on it

Maybe! Or making it come back some other way. Or something. The trouble is that we enrich the settings that the rest of the application gets with other stuff so some stuff is just missing from Environment.settings() at it isn't obvious unless you've read a bunch of the startup code.

nik9000 · 2018-10-30T12:49:43Z

...assic/src/main/java/org/elasticsearch/discovery/azure/classic/AzureUnicastHostsProvider.java

@@ -93,6 +93,7 @@ public static Deployment fromString(String string) {
        }
    }

+    private final Settings settings;


This is my "normal" pattern for this PR.

nik9000 · 2018-10-30T12:50:09Z

...assic/src/main/java/org/elasticsearch/discovery/azure/classic/AzureUnicastHostsProvider.java

@@ -108,6 +109,7 @@ public static Deployment fromString(String string) {
    public AzureUnicastHostsProvider(Settings settings, AzureComputeService azureComputeService,
                                     TransportService transportService, NetworkService networkService) {
        super(settings);


I'll drop this call and the ones like it in a follow up change, but it'll be quite large so I wanted to separate it from this one.

nik9000 · 2018-10-30T12:51:45Z

server/src/main/java/org/elasticsearch/cluster/service/ClusterApplierService.java

@@ -134,7 +134,7 @@ protected synchronized void doStart() {
        addListener(localNodeMasterListeners);
        threadPoolExecutor = EsExecutors.newSinglePrioritizing(
                nodeName + "/" + CLUSTER_UPDATE_THREAD_NAME,
-                daemonThreadFactory(settings, CLUSTER_UPDATE_THREAD_NAME),
+                daemonThreadFactory(nodeName, CLUSTER_UPDATE_THREAD_NAME),


I added this method to EsExecutors because I like how explicit it is. We already have the node name in a few places and don't have settings in this one.

nik9000 · 2018-10-30T12:52:31Z

server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java

@@ -26,10 +26,8 @@
 public abstract class AbstractComponent {

    protected final Logger logger;
-    protected final Settings settings;

    public AbstractComponent(Settings settings) {


I'm intentionally keeping the settings argument because removing it is bigger than this change. We do much more plumbing of this object then we do using it.

nik9000 · 2018-10-30T12:52:57Z

server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

@@ -50,21 +50,24 @@
 public abstract class AbstractScopedSettings extends AbstractComponent {
    public static final String ARCHIVED_SETTINGS_PREFIX = "archived.";
    private Settings lastSettingsApplied = Settings.EMPTY;
+    private static final Pattern KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])*[-\\w]+$");
+    private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+$");


These were out of order.

nik9000 · 2018-10-30T12:54:08Z

qa/smoke-test-http/src/test/java/org/elasticsearch/http/TestDeprecationHeaderRestAction.java

@@ -58,7 +58,7 @@
        Setting.boolSetting("test.setting.not_deprecated", false,
                            Setting.Property.NodeScope, Setting.Property.Dynamic);

-    private static final Map<String, Setting<?>> SETTINGS;
+    private static final Map<String, Setting<?>> SETTINGS_MAP;


I didn't want to have SETTINGS and settings so I renamed this.

nik9000 · 2018-10-30T13:53:27Z

Failure looks real. Oops!

jaymode

Nice. I left a few questions/comments

jaymode · 2018-10-30T16:33:35Z

server/src/main/java/org/elasticsearch/action/admin/indices/analyze/TransportAnalyzeAction.java

@@ -163,7 +163,7 @@ protected AnalyzeResponse shardOperation(AnalyzeRequest request, ShardId shardId
            }
            final AnalysisRegistry analysisRegistry = indicesService.getAnalysis();
            final int maxTokenCount = indexService == null ?
-                IndexSettings.MAX_TOKEN_COUNT_SETTING.get(settings) : indexService.getIndexSettings().getMaxTokenCount();
+                IndexSettings.MAX_TOKEN_COUNT_SETTING.get(Settings.EMPTY) : indexService.getIndexSettings().getMaxTokenCount();


why Settings.EMPTY here?

Ah! Good catch. I was going to leave a comment about it on the PR but forgot. I think I'll go with using settings here instead of this.

So you can specify index.analyze.max_token_count in elasticsearch.yml. Or on an index. I'm not sure why. But you can. When I made this change I thought "there is no way we document this so we don't need to support reading it from elasticsearch.yml" but I think that is not a good change to be sneaking into this.

jaymode · 2018-10-30T16:36:19Z

server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

-    private static final Pattern KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])*[-\\w]+$");
-    private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+$");
-    private static final Pattern AFFIX_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+[*](?:[.][-\\w]+)+$");
+    private Settings lastSettingsApplied = Settings.EMPTY;


I don't think we need the assignment here since it is assigned in the constructor?

I was just moving it around but I see what you mean. I'll fix.

jaymode · 2018-10-30T16:39:03Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

@@ -204,6 +204,8 @@

    private static final String DATA_BLOB_PREFIX = "__";

+    private final Settings globalSettings;


can we just call this settings?

The rest of the class calls it globalSettings. I'll just rename it everywhere. So the class lines up with the rest of our classes.

jaymode · 2018-10-30T16:40:45Z

...ty/src/main/java/org/elasticsearch/xpack/security/action/user/TransportDeleteUserAction.java

@@ -25,13 +25,15 @@

 public class TransportDeleteUserAction extends HandledTransportAction<DeleteUserRequest, DeleteUserResponse> {

+    private Settings settings;


make it final?

nik9000 · 2018-10-30T18:37:17Z

@jaymode, I pushed a commit that fixes what you asked for and left comments for a few other things.

jaymode

LGTM!

nik9000 · 2018-10-30T19:08:37Z

Thanks @jaymode!

Drops the `Settings` member from `AbstractComponent`, moving it from the base class on to the classes that use it. For the most part this is a mechanical change that doesn't drop `Settings` accesses. The one exception to this is naming threads where it switches from an invocation that passes `Settings` and extracts the node name to one that explicitly passes the node name. This change doesn't drop the `Settings` argument from `AbstractComponent`'s ctor because this change is big enough as is. We'll do that in a follow up change.

The previous merge introduced semantic conflicts that were not resolved at merge time, particularly due to PRs elastic#35083, elastic#35140 and elastic#35172. This fixes that.

nik9000 added :Core/Infra/Core Core issues without another label >breaking-java v7.0.0 v6.6.0 labels Oct 30, 2018

nik9000 requested review from rjernst and jaymode October 30, 2018 12:48

Fix order

11092e0

nik9000 commented Oct 30, 2018

View reviewed changes

nik9000 added 2 commits October 30, 2018 11:59

Merge branch 'master' into abstract_component_no_settings

c2ce7be

Sneaky sneaky

378b411

jaymode reviewed Oct 30, 2018

View reviewed changes

nik9000 added 2 commits October 30, 2018 12:45

Add node name to test cluster services

c6ec0d0

Cleanup

d0c059c

jaymode approved these changes Oct 30, 2018

View reviewed changes

nik9000 merged commit 086ada4 into elastic:master Oct 30, 2018

nik9000 added backport pending and removed backport pending labels Oct 30, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Drop settings member from AbstractComponent #35083

Core: Drop settings member from AbstractComponent #35083

nik9000 commented Oct 30, 2018

elasticmachine commented Oct 30, 2018

nik9000 Oct 30, 2018

jaymode Oct 30, 2018

nik9000 Oct 30, 2018

nik9000 Oct 30, 2018

nik9000 Oct 30, 2018

nik9000 Oct 30, 2018

nik9000 Oct 30, 2018

nik9000 Oct 30, 2018

nik9000 Oct 30, 2018

nik9000 commented Oct 30, 2018

jaymode left a comment

jaymode Oct 30, 2018

nik9000 Oct 30, 2018

jaymode Oct 30, 2018

nik9000 Oct 30, 2018

jaymode Oct 30, 2018

nik9000 Oct 30, 2018

jaymode Oct 30, 2018

nik9000 Oct 30, 2018

nik9000 commented Oct 30, 2018

jaymode left a comment

nik9000 commented Oct 30, 2018

		@@ -204,6 +204,8 @@

		private static final String DATA_BLOB_PREFIX = "__";

		private final Settings globalSettings;

		@@ -25,13 +25,15 @@

		public class TransportDeleteUserAction extends HandledTransportAction<DeleteUserRequest, DeleteUserResponse> {

		private Settings settings;

Core: Drop settings member from AbstractComponent #35083

Core: Drop settings member from AbstractComponent #35083

Conversation

nik9000 commented Oct 30, 2018

elasticmachine commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Oct 30, 2018

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Oct 30, 2018

jaymode left a comment

Choose a reason for hiding this comment

nik9000 commented Oct 30, 2018