From dbbfadeefaffa71c46a7640437524779be611bcb Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 1 Jul 2016 09:36:44 -0700 Subject: [PATCH] Internal: Remove cyclic dependency between HttpServer and NodeService NodeService has an "service attributes" map, which is only set by HttpServer on start/stop. But the only thing it puts in this map is already available as part of the HttpServer info which is added to node info requests. This change removes the attributes map and removes the dependency in HttpServer on NodeService. --- .../admin/cluster/node/info/NodeInfo.java | 38 ++++--------- .../cluster/node/info/NodesInfoResponse.java | 6 -- .../org/elasticsearch/http/HttpServer.java | 8 +-- .../node/service/NodeService.java | 56 ++++--------------- .../nodesinfo/NodeInfoStreamingTests.java | 8 +-- 5 files changed, 26 insertions(+), 90 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodeInfo.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodeInfo.java index 64292cb9064..685ab703fbc 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodeInfo.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodeInfo.java @@ -46,8 +46,6 @@ import static java.util.Collections.unmodifiableMap; * Node information (static, does not change over time). */ public class NodeInfo extends BaseNodeResponse { - @Nullable - private Map serviceAttributes; private Version version; private Build build; @@ -85,14 +83,13 @@ public class NodeInfo extends BaseNodeResponse { public NodeInfo() { } - public NodeInfo(Version version, Build build, DiscoveryNode node, @Nullable Map serviceAttributes, @Nullable Settings settings, + public NodeInfo(Version version, Build build, DiscoveryNode node, @Nullable Settings settings, @Nullable OsInfo os, @Nullable ProcessInfo process, @Nullable JvmInfo jvm, @Nullable ThreadPoolInfo threadPool, @Nullable TransportInfo transport, @Nullable HttpInfo http, @Nullable PluginsAndModules plugins, @Nullable IngestInfo ingest, @Nullable ByteSizeValue totalIndexingBuffer) { super(node); this.version = version; this.build = build; - this.serviceAttributes = serviceAttributes; this.settings = settings; this.os = os; this.process = process; @@ -127,14 +124,6 @@ public class NodeInfo extends BaseNodeResponse { return this.build; } - /** - * The service attributes of the node. - */ - @Nullable - public Map getServiceAttributes() { - return this.serviceAttributes; - } - /** * The settings of the node. */ @@ -213,13 +202,15 @@ public class NodeInfo extends BaseNodeResponse { } else { totalIndexingBuffer = null; } - if (in.readBoolean()) { - Map builder = new HashMap<>(); - int size = in.readVInt(); - for (int i = 0; i < size; i++) { - builder.put(in.readString(), in.readString()); + if (version.onOrBefore(Version.V_5_0_0_alpha4)) { + // service attributes were removed + if (in.readBoolean()) { + int size = in.readVInt(); + for (int i = 0; i < size; i++) { + in.readString(); // key + in.readString(); // value + } } - serviceAttributes = unmodifiableMap(builder); } if (in.readBoolean()) { settings = Settings.readSettingsFromStream(in); @@ -262,15 +253,8 @@ public class NodeInfo extends BaseNodeResponse { out.writeBoolean(true); out.writeLong(totalIndexingBuffer.bytes()); } - if (getServiceAttributes() == null) { - out.writeBoolean(false); - } else { - out.writeBoolean(true); - out.writeVInt(serviceAttributes.size()); - for (Map.Entry entry : serviceAttributes.entrySet()) { - out.writeString(entry.getKey()); - out.writeString(entry.getValue()); - } + if (version.onOrBefore(Version.V_5_0_0_alpha4)) { + out.writeBoolean(false); // service attributes removed } if (settings == null) { out.writeBoolean(false); diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java index a9dc0ff2e45..2d273bef2c0 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java @@ -73,12 +73,6 @@ public class NodesInfoResponse extends BaseNodesResponse implements To builder.byteSizeField("total_indexing_buffer", "total_indexing_buffer_in_bytes", nodeInfo.getTotalIndexingBuffer()); } - if (nodeInfo.getServiceAttributes() != null) { - for (Map.Entry nodeAttribute : nodeInfo.getServiceAttributes().entrySet()) { - builder.field(nodeAttribute.getKey(), nodeAttribute.getValue()); - } - } - builder.startArray("roles"); for (DiscoveryNode.Role role : nodeInfo.getNode().getRoles()) { builder.value(role.getRoleName()); diff --git a/core/src/main/java/org/elasticsearch/http/HttpServer.java b/core/src/main/java/org/elasticsearch/http/HttpServer.java index f44b839b2a3..63bcd761eb3 100644 --- a/core/src/main/java/org/elasticsearch/http/HttpServer.java +++ b/core/src/main/java/org/elasticsearch/http/HttpServer.java @@ -56,22 +56,18 @@ public class HttpServer extends AbstractLifecycleComponent implement private final RestController restController; - private final NodeService nodeService; - private final NodeClient client; private final CircuitBreakerService circuitBreakerService; @Inject - public HttpServer(Settings settings, HttpServerTransport transport, RestController restController, NodeService nodeService, + public HttpServer(Settings settings, HttpServerTransport transport, RestController restController, NodeClient client, CircuitBreakerService circuitBreakerService) { super(settings); this.transport = transport; this.restController = restController; - this.nodeService = nodeService; this.client = client; this.circuitBreakerService = circuitBreakerService; - nodeService.setHttpServer(this); transport.httpServerAdapter(this); } @@ -82,12 +78,10 @@ public class HttpServer extends AbstractLifecycleComponent implement if (logger.isInfoEnabled()) { logger.info("{}", transport.boundAddress()); } - nodeService.putAttribute("http_address", transport.boundAddress().publishAddress().toString()); } @Override protected void doStop() { - nodeService.removeAttribute("http_address"); transport.stop(); } diff --git a/core/src/main/java/org/elasticsearch/node/service/NodeService.java b/core/src/main/java/org/elasticsearch/node/service/NodeService.java index 0920a84bfd6..f3743a46d1e 100644 --- a/core/src/main/java/org/elasticsearch/node/service/NodeService.java +++ b/core/src/main/java/org/elasticsearch/node/service/NodeService.java @@ -19,13 +19,15 @@ package org.elasticsearch.node.service; +import java.io.Closeable; +import java.io.IOException; + import org.elasticsearch.Build; import org.elasticsearch.Version; import org.elasticsearch.action.admin.cluster.node.info.NodeInfo; import org.elasticsearch.action.admin.cluster.node.stats.NodeStats; import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags; import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.Nullable; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; @@ -42,14 +44,6 @@ import org.elasticsearch.script.ScriptService; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; -import java.io.Closeable; -import java.io.IOException; -import java.util.HashMap; -import java.util.Map; - -import static java.util.Collections.emptyMap; -import static java.util.Collections.unmodifiableMap; - /** */ public class NodeService extends AbstractComponent implements Closeable { @@ -64,18 +58,14 @@ public class NodeService extends AbstractComponent implements Closeable { private final SettingsFilter settingsFilter; private ClusterService clusterService; private ScriptService scriptService; - - @Nullable - private HttpServer httpServer; - - private volatile Map serviceAttributes = emptyMap(); + private final HttpServer httpServer; private final Discovery discovery; @Inject public NodeService(Settings settings, ThreadPool threadPool, MonitorService monitorService, Discovery discovery, TransportService transportService, IndicesService indicesService, - PluginsService pluginService, CircuitBreakerService circuitBreakerService, + PluginsService pluginService, CircuitBreakerService circuitBreakerService, HttpServer httpServer, ProcessorsRegistry.Builder processorsRegistryBuilder, ClusterService clusterService, SettingsFilter settingsFilter) { super(settings); this.threadPool = threadPool; @@ -85,6 +75,7 @@ public class NodeService extends AbstractComponent implements Closeable { this.discovery = discovery; this.pluginService = pluginService; this.circuitBreakerService = circuitBreakerService; + this.httpServer = httpServer; this.clusterService = clusterService; this.ingestService = new IngestService(settings, threadPool, processorsRegistryBuilder); this.settingsFilter = settingsFilter; @@ -99,38 +90,15 @@ public class NodeService extends AbstractComponent implements Closeable { this.ingestService.buildProcessorsFactoryRegistry(scriptService, clusterService); } - public void setHttpServer(@Nullable HttpServer httpServer) { - this.httpServer = httpServer; - } - - public synchronized void putAttribute(String key, String value) { - Map newServiceAttributes = new HashMap<>(serviceAttributes); - newServiceAttributes.put(key, value); - serviceAttributes = unmodifiableMap(newServiceAttributes); - } - - public synchronized void removeAttribute(String key) { - Map newServiceAttributes = new HashMap<>(serviceAttributes); - newServiceAttributes.remove(key); - serviceAttributes = unmodifiableMap(newServiceAttributes); - } - - /** - * Attributes different services in the node can add to be reported as part of the node info (for example). - */ - public Map attributes() { - return this.serviceAttributes; - } - public NodeInfo info() { - return new NodeInfo(Version.CURRENT, Build.CURRENT, discovery.localNode(), serviceAttributes, + return new NodeInfo(Version.CURRENT, Build.CURRENT, discovery.localNode(), settings, monitorService.osService().info(), monitorService.processService().info(), monitorService.jvmService().info(), threadPool.info(), transportService.info(), - httpServer == null ? null : httpServer.info(), + httpServer.info(), pluginService == null ? null : pluginService.info(), ingestService == null ? null : ingestService.info(), indicesService.getTotalIndexingBufferBytes() @@ -139,14 +107,14 @@ public class NodeService extends AbstractComponent implements Closeable { public NodeInfo info(boolean settings, boolean os, boolean process, boolean jvm, boolean threadPool, boolean transport, boolean http, boolean plugin, boolean ingest, boolean indices) { - return new NodeInfo(Version.CURRENT, Build.CURRENT, discovery.localNode(), serviceAttributes, + return new NodeInfo(Version.CURRENT, Build.CURRENT, discovery.localNode(), settings ? settingsFilter.filter(this.settings) : null, os ? monitorService.osService().info() : null, process ? monitorService.processService().info() : null, jvm ? monitorService.jvmService().info() : null, threadPool ? this.threadPool.info() : null, transport ? transportService.info() : null, - http ? (httpServer == null ? null : httpServer.info()) : null, + http ? httpServer.info() : null, plugin ? (pluginService == null ? null : pluginService.info()) : null, ingest ? (ingestService == null ? null : ingestService.info()) : null, indices ? indicesService.getTotalIndexingBufferBytes() : null @@ -164,7 +132,7 @@ public class NodeService extends AbstractComponent implements Closeable { threadPool.stats(), monitorService.fsService().stats(), transportService.stats(), - httpServer == null ? null : httpServer.stats(), + httpServer.stats(), circuitBreakerService.stats(), scriptService.stats(), discovery.stats(), @@ -185,7 +153,7 @@ public class NodeService extends AbstractComponent implements Closeable { threadPool ? this.threadPool.stats() : null, fs ? monitorService.fsService().stats() : null, transport ? transportService.stats() : null, - http ? (httpServer == null ? null : httpServer.stats()) : null, + http ? httpServer.stats() : null, circuitBreaker ? circuitBreakerService.stats() : null, script ? scriptService.stats() : null, discoveryStats ? discovery.stats() : null, diff --git a/core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java b/core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java index ea82a17f7b3..739dcd8b2c6 100644 --- a/core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java +++ b/core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java @@ -81,11 +81,6 @@ public class NodeInfoStreamingTests extends ESTestCase { assertThat(nodeInfo.getBuild().toString(), equalTo(readNodeInfo.getBuild().toString())); assertThat(nodeInfo.getHostname(), equalTo(readNodeInfo.getHostname())); assertThat(nodeInfo.getVersion(), equalTo(readNodeInfo.getVersion())); - assertThat(nodeInfo.getServiceAttributes().size(), equalTo(readNodeInfo.getServiceAttributes().size())); - for (Map.Entry entry : nodeInfo.getServiceAttributes().entrySet()) { - assertNotNull(readNodeInfo.getServiceAttributes().get(entry.getKey())); - assertThat(readNodeInfo.getServiceAttributes().get(entry.getKey()), equalTo(entry.getValue())); - } compareJsonOutput(nodeInfo.getHttp(), readNodeInfo.getHttp()); compareJsonOutput(nodeInfo.getJvm(), readNodeInfo.getJvm()); compareJsonOutput(nodeInfo.getProcess(), readNodeInfo.getProcess()); @@ -149,6 +144,7 @@ public class NodeInfoStreamingTests extends ESTestCase { // pick a random long that sometimes exceeds an int: indexingBuffer = new ByteSizeValue(random().nextLong() & ((1L<<40)-1)); } - return new NodeInfo(VersionUtils.randomVersion(random()), build, node, serviceAttributes, settings, osInfo, process, jvm, threadPoolInfo, transport, htttpInfo, plugins, ingestInfo, indexingBuffer); + return new NodeInfo(VersionUtils.randomVersion(random()), build, node, settings, osInfo, process, jvm, + threadPoolInfo, transport, htttpInfo, plugins, ingestInfo, indexingBuffer); } }