Skip to content
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.

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

Core: Drop settings member from AbstractComponent #35083

Merged
merged 6 commits into from
Oct 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

throw new RepositoryException(metadata.name(), "missing url");
}
this.environment = environment;
supportedProtocols = SUPPORTED_PROTOCOLS_SETTING.get(settings);
urlWhiteList = ALLOWED_URLS_SETTING.get(settings).toArray(new URIPattern[]{});
supportedProtocols = SUPPORTED_PROTOCOLS_SETTING.get(environment.settings());
urlWhiteList = ALLOWED_URLS_SETTING.get(environment.settings()).toArray(new URIPattern[]{});
basePath = BlobPath.cleanPath();
url = URL_SETTING.exists(metadata.settings())
? URL_SETTING.get(metadata.settings()) : REPOSITORIES_URL_SETTING.get(settings);
? URL_SETTING.get(metadata.settings()) : REPOSITORIES_URL_SETTING.get(environment.settings());
}

@Override
protected BlobStore createBlobStore() {
URL normalizedURL = checkURL(url);
return new URLBlobStore(settings, normalizedURL);
return new URLBlobStore(environment.settings(), normalizedURL);
}

// only use for testing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public static Deployment fromString(String string) {
}
}

private final Settings settings;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is my "normal" pattern for this PR.

private final AzureComputeService azureComputeService;
private TransportService transportService;
private NetworkService networkService;
Expand All @@ -108,6 +109,7 @@ public static Deployment fromString(String string) {
public AzureUnicastHostsProvider(Settings settings, AzureComputeService azureComputeService,
TransportService transportService, NetworkService networkService) {
super(settings);
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

this.settings = settings;
this.azureComputeService = azureComputeService;
this.transportService = transportService;
this.networkService = networkService;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ public Collection<Instance> instances() {
return instances;
}

private final Settings settings;
private Compute client;
private TimeValue refreshInterval = null;
private long lastRefresh;
Expand All @@ -108,6 +109,7 @@ public Collection<Instance> instances() {

public GceInstancesServiceImpl(Settings settings) {
super(settings);
this.settings = settings;
this.validateCerts = GCE_VALIDATE_CERTIFICATES.get(settings);
this.project = resolveProject();
this.zones = resolveZones();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,14 @@ public class GceMetadataService extends AbstractLifecycleComponent {
public static final Setting<String> GCE_HOST =
new Setting<>("cloud.gce.host", "http://metadata.google.internal", Function.identity(), Setting.Property.NodeScope);

private final Settings settings;

/** Global instance of the HTTP transport. */
private HttpTransport gceHttpTransport;

public GceMetadataService(Settings settings) {
super(settings);
this.settings = settings;
}

protected synchronized HttpTransport getGceHttpTransport() throws GeneralSecurityException, IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ static final class Status {
private static final String TERMINATED = "TERMINATED";
}

private final Settings settings;
private final GceInstancesService gceInstancesService;
private TransportService transportService;
private NetworkService networkService;
Expand All @@ -74,6 +75,7 @@ public GceUnicastHostsProvider(Settings settings, GceInstancesService gceInstanc
TransportService transportService,
NetworkService networkService) {
super(settings);
this.settings = settings;
this.gceInstancesService = gceInstancesService;
this.transportService = transportService;
this.networkService = networkService;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.blobstore.BlobPath;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
Expand Down Expand Up @@ -55,6 +56,7 @@ class GoogleCloudStorageRepository extends BlobStoreRepository {
byteSizeSetting("chunk_size", MAX_CHUNK_SIZE, MIN_CHUNK_SIZE, MAX_CHUNK_SIZE, Property.NodeScope, Property.Dynamic);
static final Setting<String> CLIENT_NAME = new Setting<>("client", "default", Function.identity());

private final Settings settings;
private final GoogleCloudStorageService storageService;
private final BlobPath basePath;
private final boolean compress;
Expand All @@ -66,6 +68,7 @@ class GoogleCloudStorageRepository extends BlobStoreRepository {
NamedXContentRegistry namedXContentRegistry,
GoogleCloudStorageService storageService) {
super(metadata, environment.settings(), namedXContentRegistry);
this.settings = environment.settings();
this.storageService = storageService;

String basePath = BASE_PATH.get(metadata.settings());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ class S3Repository extends BlobStoreRepository {
*/
static final Setting<String> BASE_PATH_SETTING = Setting.simpleString("base_path");

private final Settings settings;

private final S3Service service;

private final String bucket;
Expand Down Expand Up @@ -178,6 +180,7 @@ class S3Repository extends BlobStoreRepository {
final NamedXContentRegistry namedXContentRegistry,
final S3Service service) {
super(metadata, settings, namedXContentRegistry);
this.settings = settings;
this.service = service;

// Parse and validate the user's S3 Storage Class setting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public class TestDeprecationHeaderRestAction extends BaseRestHandler {
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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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


static {
Map<String, Setting<?>> settingsMap = new HashMap<>(3);
Expand All @@ -67,14 +67,17 @@ public class TestDeprecationHeaderRestAction extends BaseRestHandler {
settingsMap.put(TEST_DEPRECATED_SETTING_TRUE2.getKey(), TEST_DEPRECATED_SETTING_TRUE2);
settingsMap.put(TEST_NOT_DEPRECATED_SETTING.getKey(), TEST_NOT_DEPRECATED_SETTING);

SETTINGS = Collections.unmodifiableMap(settingsMap);
SETTINGS_MAP = Collections.unmodifiableMap(settingsMap);
}

public static final String DEPRECATED_ENDPOINT = "[/_test_cluster/deprecated_settings] exists for deprecated tests";
public static final String DEPRECATED_USAGE = "[deprecated_settings] usage is deprecated. use [settings] instead";

private final Settings settings;

public TestDeprecationHeaderRestAction(Settings settings, RestController controller) {
super(settings);
this.settings = settings;

controller.registerAsDeprecatedHandler(RestRequest.Method.GET, "/_test_cluster/deprecated_settings", this,
DEPRECATED_ENDPOINT, deprecationLogger);
Expand Down Expand Up @@ -107,7 +110,7 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client

builder.startObject().startArray("settings");
for (String setting : settings) {
builder.startObject().field(setting, SETTINGS.get(setting).getRaw(this.settings)).endObject();
builder.startObject().field(setting, SETTINGS_MAP.get(setting).getRaw(this.settings)).endObject();
}
builder.endArray().endObject();
channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
*/
public class TransportAnalyzeAction extends TransportSingleShardAction<AnalyzeRequest, AnalyzeResponse> {

private final Settings settings;
private final IndicesService indicesService;
private final Environment environment;

Expand All @@ -88,6 +89,7 @@ public TransportAnalyzeAction(Settings settings, ThreadPool threadPool, ClusterS
IndexNameExpressionResolver indexNameExpressionResolver, Environment environment) {
super(settings, AnalyzeAction.NAME, threadPool, clusterService, transportService, actionFilters, indexNameExpressionResolver,
AnalyzeRequest::new, ThreadPool.Names.ANALYZE);
this.settings = settings;
this.indicesService = indicesService;
this.environment = environment;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public TransportShardBulkAction(Settings settings, TransportService transportSer
}

@Override
protected TransportRequestOptions transportOptions() {
protected TransportRequestOptions transportOptions(Settings settings) {
return BulkAction.INSTANCE.transportOptions(settings);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,22 @@

public class TransportMainAction extends HandledTransportAction<MainRequest, MainResponse> {

private final String nodeName;
private final ClusterService clusterService;

@Inject
public TransportMainAction(Settings settings, TransportService transportService,
ActionFilters actionFilters, ClusterService clusterService) {
super(settings, MainAction.NAME, transportService, actionFilters, MainRequest::new);
this.nodeName = Node.NODE_NAME_SETTING.get(settings);
this.clusterService = clusterService;
}

@Override
protected void doExecute(Task task, MainRequest request, ActionListener<MainResponse> listener) {
ClusterState clusterState = clusterService.state();
assert Node.NODE_NAME_SETTING.exists(settings);
listener.onResponse(
new MainResponse(Node.NODE_NAME_SETTING.get(settings), Version.CURRENT, clusterState.getClusterName(),
new MainResponse(nodeName, Version.CURRENT, clusterState.getClusterName(),
clusterState.metaData().clusterUUID(), Build.CURRENT));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ protected TransportReplicationAction(Settings settings, String actionName, Trans
this.transportReplicaAction = actionName + "[r]";
registerRequestHandlers(actionName, transportService, request, replicaRequest, executor);

this.transportOptions = transportOptions();
this.transportOptions = transportOptions(settings);

this.syncGlobalCheckpointAfterOperation = syncGlobalCheckpointAfterOperation;
}
Expand Down Expand Up @@ -231,7 +231,7 @@ protected boolean resolveIndex() {
return true;
}

protected TransportRequestOptions transportOptions() {
protected TransportRequestOptions transportOptions(Settings settings) {
return TransportRequestOptions.EMPTY;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,12 +344,14 @@

public abstract class AbstractClient extends AbstractComponent implements Client {

protected final Settings settings;
private final ThreadPool threadPool;
private final Admin admin;
private final ThreadedActionListener.Wrapper threadedWrapper;

public AbstractClient(Settings settings, ThreadPool threadPool) {
super(settings);
this.settings = settings;
this.threadPool = threadPool;
this.admin = new Admin(this);
this.threadedWrapper = new ThreadedActionListener.Wrapper(logger, settings, threadPool);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,15 +130,15 @@ final class TransportClientNodesService extends AbstractComponent implements Clo
this.threadPool = threadPool;
this.minCompatibilityVersion = Version.CURRENT.minimumCompatibilityVersion();

this.nodesSamplerInterval = TransportClient.CLIENT_TRANSPORT_NODES_SAMPLER_INTERVAL.get(this.settings);
this.pingTimeout = TransportClient.CLIENT_TRANSPORT_PING_TIMEOUT.get(this.settings).millis();
this.ignoreClusterName = TransportClient.CLIENT_TRANSPORT_IGNORE_CLUSTER_NAME.get(this.settings);
this.nodesSamplerInterval = TransportClient.CLIENT_TRANSPORT_NODES_SAMPLER_INTERVAL.get(settings);
this.pingTimeout = TransportClient.CLIENT_TRANSPORT_PING_TIMEOUT.get(settings).millis();
this.ignoreClusterName = TransportClient.CLIENT_TRANSPORT_IGNORE_CLUSTER_NAME.get(settings);

if (logger.isDebugEnabled()) {
logger.debug("node_sampler_interval[{}]", nodesSamplerInterval);
}

if (TransportClient.CLIENT_TRANSPORT_SNIFF.get(this.settings)) {
if (TransportClient.CLIENT_TRANSPORT_SNIFF.get(settings)) {
this.nodesSampler = new SniffNodesSampler();
} else {
this.nodesSampler = new SimpleNodeSampler();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ public class MetaDataCreateIndexService extends AbstractComponent {

public static final int MAX_INDEX_NAME_BYTES = 255;

private final Settings settings;
private final ClusterService clusterService;
private final IndicesService indicesService;
private final AllocationService allocationService;
Expand All @@ -128,6 +129,7 @@ public MetaDataCreateIndexService(
final NamedXContentRegistry xContentRegistry,
final boolean forbidPrivateIndexSettings) {
super(settings);
this.settings = settings;
this.clusterService = clusterService;
this.indicesService = indicesService;
this.allocationService = allocationService;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,15 @@
* Deletes indices.
*/
public class MetaDataDeleteIndexService extends AbstractComponent {

private final Settings settings;
private final ClusterService clusterService;

private final AllocationService allocationService;

@Inject
public MetaDataDeleteIndexService(Settings settings, ClusterService clusterService, AllocationService allocationService) {
super(settings);
this.settings = settings;
this.clusterService = clusterService;
this.allocationService = allocationService;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
*/
public class MetaDataIndexUpgradeService extends AbstractComponent {

private final Settings settings;
private final NamedXContentRegistry xContentRegistry;
private final MapperRegistry mapperRegistry;
private final IndexScopedSettings indexScopedSettings;
Expand All @@ -62,6 +63,7 @@ public MetaDataIndexUpgradeService(Settings settings, NamedXContentRegistry xCon
IndexScopedSettings indexScopedSettings,
Collection<UnaryOperator<IndexMetaData>> indexMetaDataUpgraders) {
super(settings);
this.settings = settings;
this.xContentRegistry = xContentRegistry;
this.mapperRegistry = mapperRegistry;
this.indexScopedSettings = indexScopedSettings;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,11 @@ public class ShardsLimitAllocationDecider extends AllocationDecider {
Setting.intSetting("cluster.routing.allocation.total_shards_per_node", -1, -1,
Property.Dynamic, Property.NodeScope);

private final Settings settings;

public ShardsLimitAllocationDecider(Settings settings, ClusterSettings clusterSettings) {
super(settings);
this.settings = settings;
this.clusterShardLimit = CLUSTER_TOTAL_SHARDS_PER_NODE_SETTING.get(settings);
clusterSettings.addSettingsUpdateConsumer(CLUSTER_TOTAL_SHARDS_PER_NODE_SETTING, this::setClusterShardLimit);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

threadPool.getThreadContext(),
threadPool.scheduler());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ public class ClusterService extends AbstractLifecycleComponent {
public static final org.elasticsearch.common.settings.Setting.AffixSetting<String> USER_DEFINED_META_DATA =
Setting.prefixKeySetting("cluster.metadata.", (key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope));

/**
* The node's settings.
*/
private final Settings settings;

private final ClusterName clusterName;

private final OperationRouting operationRouting;
Expand All @@ -65,6 +70,7 @@ public class ClusterService extends AbstractLifecycleComponent {

public ClusterService(Settings settings, ClusterSettings clusterSettings, ThreadPool threadPool) {
super(settings);
this.settings = settings;
this.nodeName = Node.NODE_NAME_SETTING.get(settings);
this.masterService = new MasterService(nodeName, settings, threadPool);
this.operationRouting = new OperationRouting(settings, clusterSettings);
Expand Down Expand Up @@ -199,6 +205,9 @@ public ClusterSettings getClusterSettings() {
return clusterSettings;
}

/**
* The node's settings.
*/
public Settings getSettings() {
return settings;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ protected synchronized void doStart() {
Objects.requireNonNull(clusterStateSupplier, "please set a cluster state supplier before starting");
threadPoolExecutor = EsExecutors.newSinglePrioritizing(
nodeName + "/" + MASTER_UPDATE_THREAD_NAME,
daemonThreadFactory(settings, MASTER_UPDATE_THREAD_NAME),
daemonThreadFactory(nodeName, MASTER_UPDATE_THREAD_NAME),
threadPool.getThreadContext(),
threadPool.scheduler());
taskBatcher = new Batcher(logger, threadPoolExecutor);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,8 @@
public abstract class AbstractComponent {

protected final Logger logger;
protected final Settings settings;

public AbstractComponent(Settings settings) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

this.logger = LogManager.getLogger(getClass());
this.settings = settings;
}
}