Skip to content

Internal: Remove cyclic dependency between HttpServer and NodeService #19218

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

Merged
rjernst merged 2 commits into from
Jul 1, 2016
Merged

Internal: Remove cyclic dependency between HttpServer and NodeService #19218

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbbfade
Internal: Remove cyclic dependency between HttpServer and NodeService
rjernst Jul 1, 2016
a206c7a
Remove unnecessary transport level bwc
rjernst Jul 1, 2016
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
38 changes: 11 additions & 27 deletions core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodeInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@
* Node information (static, does not change over time).
*/
public class NodeInfo extends BaseNodeResponse {
@Nullable
private Map<String, String> serviceAttributes;

private Version version;
private Build build;
Expand Down Expand Up @@ -85,14 +83,13 @@ public class NodeInfo extends BaseNodeResponse {
public NodeInfo() {
}

public NodeInfo(Version version, Build build, DiscoveryNode node, @Nullable Map<String, String> 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;
Expand Down Expand Up @@ -127,14 +124,6 @@ public Build getBuild() {
return this.build;
}

/**
* The service attributes of the node.
*/
@Nullable
public Map<String, String> getServiceAttributes() {
return this.serviceAttributes;
}

/**
* The settings of the node.
*/
Expand Down Expand Up @@ -213,13 +202,15 @@ public void readFrom(StreamInput in) throws IOException {
} else {
totalIndexingBuffer = null;
}
if (in.readBoolean()) {
Map<String, String> 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)) {

@javanna javanna Jul 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need this bw comp layer?

@rjernst rjernst Jul 1, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are right, we don't! I will remove.

// 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);
Expand Down Expand Up @@ -262,15 +253,8 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeBoolean(true);
out.writeLong(totalIndexingBuffer.bytes());
}
if (getServiceAttributes() == null) {
out.writeBoolean(false);
} else {
out.writeBoolean(true);
out.writeVInt(serviceAttributes.size());
for (Map.Entry<String, String> 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);
Expand Down
6 changes: 0 additions & 6 deletions core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.byteSizeField("total_indexing_buffer", "total_indexing_buffer_in_bytes", nodeInfo.getTotalIndexingBuffer());
}

if (nodeInfo.getServiceAttributes() != null) {
for (Map.Entry<String, String> nodeAttribute : nodeInfo.getServiceAttributes().entrySet()) {
builder.field(nodeAttribute.getKey(), nodeAttribute.getValue());
}
}

builder.startArray("roles");

@javanna javanna Jul 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess this piece of info that is not returned by nodes info was already returned in some other section of nodes info as well? I think we have a proper http section for it that is still there?

@rjernst rjernst Jul 1, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes that is right.

for (DiscoveryNode.Role role : nodeInfo.getNode().getRoles()) {
builder.value(role.getRoleName());
Expand Down
8 changes: 1 addition & 7 deletions core/src/main/java/org/elasticsearch/http/HttpServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,22 +56,18 @@ public class HttpServer extends AbstractLifecycleComponent<HttpServer> 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);
}

Expand All @@ -82,12 +78,10 @@ protected void doStart() {
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();
}

Expand Down
56 changes: 12 additions & 44 deletions core/src/main/java/org/elasticsearch/node/service/NodeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -42,14 +44,6 @@
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 {
Expand All @@ -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<String, String> 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;
Expand All @@ -85,6 +75,7 @@ public NodeService(Settings settings, ThreadPool threadPool, MonitorService moni
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;
Expand All @@ -99,38 +90,15 @@ public void setScriptService(ScriptService scriptService) {
this.ingestService.buildProcessorsFactoryRegistry(scriptService, clusterService);
}

public void setHttpServer(@Nullable HttpServer httpServer) {
this.httpServer = httpServer;
}

public synchronized void putAttribute(String key, String value) {
Map<String, String> newServiceAttributes = new HashMap<>(serviceAttributes);
newServiceAttributes.put(key, value);
serviceAttributes = unmodifiableMap(newServiceAttributes);
}

public synchronized void removeAttribute(String key) {
Map<String, String> 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<String, String> 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()
Expand All @@ -139,14 +107,14 @@ public NodeInfo info() {

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
Expand All @@ -164,7 +132,7 @@ public NodeStats stats() throws IOException {
threadPool.stats(),
monitorService.fsService().stats(),
transportService.stats(),
httpServer == null ? null : httpServer.stats(),
httpServer.stats(),
circuitBreakerService.stats(),
scriptService.stats(),
discovery.stats(),
Expand All @@ -185,7 +153,7 @@ public NodeStats stats(CommonStatsFlags indices, boolean os, boolean process, bo
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,
Expand Down
8 changes: 2 additions & 6 deletions core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,6 @@ private void assertExpectedUnchanged(NodeInfo nodeInfo, NodeInfo readNodeInfo) t
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<String, String> 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());
Expand Down Expand Up @@ -149,6 +144,7 @@ private NodeInfo createNodeInfo() {
// 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);
}
}