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

Stats to record how often the ClusterState diff mechanism is used successfully #26973

Merged
merged 33 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
95f0969
Add statistics to record how often the ClusterState diff mechanism is…
DaveCTurner Oct 11, 2017
5ed348a
Picky picky
DaveCTurner Oct 11, 2017
db71be7
No need for Fields class
DaveCTurner Oct 11, 2017
092be64
Shorten arg name
DaveCTurner Oct 11, 2017
8675140
XContent not XContentFragment
DaveCTurner Oct 11, 2017
88bfda5
Make accumulators into AtomicLongs
DaveCTurner Oct 11, 2017
d825792
Add javadoc
DaveCTurner Oct 11, 2017
f004119
Slated for inclusion in 6.1
DaveCTurner Oct 11, 2017
ec982bd
Add stats for receivers of cluster states too
DaveCTurner Oct 11, 2017
c7a0192
Nit: Diffs -> Diff
DaveCTurner Oct 11, 2017
660f20a
Misread comment - should have been ToXContentObject
DaveCTurner Oct 11, 2017
5543e88
Don't bother collecting the master-side stats at all
DaveCTurner Oct 12, 2017
67c1a22
Stick with v7 for now.
DaveCTurner Oct 12, 2017
e0f3da3
Missed one
DaveCTurner Oct 12, 2017
a9a2828
Add assertions that the stats are updated as expected
DaveCTurner Oct 12, 2017
1852f8f
Remove assertions from existing test
DaveCTurner Oct 13, 2017
34c3b5a
Add testPublishClusterStateStats()
DaveCTurner Oct 13, 2017
6af7728
Better grammar
DaveCTurner Oct 13, 2017
b935508
Add REST assertions
DaveCTurner Oct 13, 2017
68d589d
Make count fields final
DaveCTurner Oct 16, 2017
efcd20d
Count incompatible cluster state diffs rather than all of them
DaveCTurner Oct 16, 2017
eaa0fd9
Assert that the sub-fields are >= 0
DaveCTurner Oct 16, 2017
a5124e0
No need for @Nullables
DaveCTurner Oct 16, 2017
d61551f
Skip these tests until v7.x
DaveCTurner Oct 16, 2017
3dd4219
Line length oops
DaveCTurner Oct 16, 2017
b589f36
Count incompatible states received in exception handler instead
DaveCTurner Oct 17, 2017
ed570df
Shorter keys in PublishClusterStateStats#toXContent()
DaveCTurner Oct 17, 2017
322c1de
Fewer linebreaks
DaveCTurner Oct 17, 2017
840a5bd
Fixed up REST API test
DaveCTurner Oct 23, 2017
54b840a
Merge branch 'master' into 2017-10-11-discovery-stats
DaveCTurner Oct 24, 2017
1fa86b7
Formatting
DaveCTurner Oct 24, 2017
ff54a0d
Inline
DaveCTurner Oct 24, 2017
168d94a
Make getters package-private
DaveCTurner Oct 24, 2017
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
24 changes: 22 additions & 2 deletions core/src/main/java/org/elasticsearch/discovery/DiscoveryStats.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@

package org.elasticsearch.discovery;

import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.discovery.zen.PendingClusterStateStats;
import org.elasticsearch.discovery.zen.PublishClusterStateStats;

import java.io.IOException;

Expand All @@ -34,25 +36,42 @@ public class DiscoveryStats implements Writeable, ToXContentFragment {
@Nullable
private final PendingClusterStateStats queueStats;

public DiscoveryStats(PendingClusterStateStats queueStats) {
@Nullable
Copy link
Member

Choose a reason for hiding this comment

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

I do not think we need the @Nullable annotation on a private field (I question its usefulness in general).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove the one above too?

private final PublishClusterStateStats publishStats;

public DiscoveryStats(PendingClusterStateStats queueStats, PublishClusterStateStats publishStats) {
this.queueStats = queueStats;
this.publishStats = publishStats;
}

public DiscoveryStats(StreamInput in) throws IOException {
queueStats = in.readOptionalWriteable(PendingClusterStateStats::new);

if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
publishStats = in.readOptionalWriteable(PublishClusterStateStats::new);
} else {
publishStats = null;
}
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeOptionalWriteable(queueStats);

if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
out.writeOptionalWriteable(publishStats);
}
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(Fields.DISCOVERY);
if (queueStats != null ){
if (queueStats != null) {
queueStats.toXContent(builder, params);
}
if (publishStats != null) {
publishStats.toXContent(builder, params);
}
builder.endObject();
return builder;
}
Expand All @@ -64,4 +83,5 @@ static final class Fields {
public PendingClusterStateStats getQueueStats() {
return queueStats;
}
public PublishClusterStateStats getPublishStats() { return publishStats; }
Copy link
Member

Choose a reason for hiding this comment

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

Would you add a newline before this method, and wrap the method definition over multiple lines please?

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.cluster.ClusterChangedEvent;
import org.elasticsearch.cluster.ClusterName;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.ClusterStateTaskListener;
import org.elasticsearch.cluster.block.ClusterBlocks;
Expand All @@ -34,6 +33,7 @@
import org.elasticsearch.discovery.Discovery;
import org.elasticsearch.discovery.DiscoveryStats;
import org.elasticsearch.discovery.zen.PendingClusterStateStats;
import org.elasticsearch.discovery.zen.PublishClusterStateStats;
import org.elasticsearch.transport.TransportService;

import java.io.IOException;
Expand Down Expand Up @@ -94,7 +94,7 @@ public void onFailure(String source, Exception e) {

@Override
public DiscoveryStats stats() {
return new DiscoveryStats((PendingClusterStateStats) null);
return new DiscoveryStats(null, null);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ public class PublishClusterStateAction extends AbstractComponent {
public static final String SEND_ACTION_NAME = "internal:discovery/zen/publish/send";
public static final String COMMIT_ACTION_NAME = "internal:discovery/zen/publish/commit";

public PublishClusterStateStats stats() {
return new PublishClusterStateStats(fullClusterStateSentCount, clusterStateDiffSentCount, incompatibleClusterStateDiffVersionCount);
}

public interface IncomingClusterStateListener {

/**
Expand All @@ -90,6 +94,10 @@ public interface IncomingClusterStateListener {
private final IncomingClusterStateListener incomingClusterStateListener;
private final DiscoverySettings discoverySettings;

private long fullClusterStateSentCount = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

these should probably be atomic...

Copy link
Member

Choose a reason for hiding this comment

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

It's sneaky. 😇

private long clusterStateDiffSentCount = 0;
private long incompatibleClusterStateDiffVersionCount = 0;

public PublishClusterStateAction(
Settings settings,
TransportService transportService,
Expand Down Expand Up @@ -249,6 +257,7 @@ private void sendFullClusterState(ClusterState clusterState, Map<Version, BytesR
return;
}
}
fullClusterStateSentCount++;
sendClusterStateToNode(clusterState, bytes, node, publishTimeout, sendingController, false, serializedStates);
}

Expand All @@ -257,6 +266,7 @@ private void sendClusterStateDiff(ClusterState clusterState,
DiscoveryNode node, TimeValue publishTimeout, SendingController sendingController) {
BytesReference bytes = serializedDiffs.get(node.getVersion());
assert bytes != null : "failed to find serialized diff for node " + node + " of version [" + node.getVersion() + "]";
clusterStateDiffSentCount++;
sendClusterStateToNode(clusterState, bytes, node, publishTimeout, sendingController, true, serializedStates);
}

Expand Down Expand Up @@ -290,6 +300,7 @@ public void handleResponse(TransportResponse.Empty response) {
public void handleException(TransportException exp) {
if (sendDiffs && exp.unwrapCause() instanceof IncompatibleClusterStateVersionException) {
logger.debug("resending full cluster state to node {} reason {}", node, exp.getDetailedMessage());
incompatibleClusterStateDiffVersionCount++;
sendFullClusterState(clusterState, serializedStates, node, publishTimeout, sendingController);
} else {
logger.debug((org.apache.logging.log4j.util.Supplier<?>) () ->
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.discovery.zen;

import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;

import java.io.IOException;

/**
* Class encapsulating stats about the PublishClusterStateAction
*/
public class PublishClusterStateStats implements Writeable, ToXContentFragment {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want ToXContentObject here.


private final long fullClusterStateSentCount;
private final long clusterStateDiffSentCount;
private final long incompatibleClusterStateDiffVersionCount;

public PublishClusterStateStats(long fullClusterStateSentCount,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you java doc the parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Params or getters? Or both?

long clusterStateDiffSentCount,
long incompatibleClusterStateDiffVersionCount) {
this.fullClusterStateSentCount = fullClusterStateSentCount;
this.clusterStateDiffSentCount = clusterStateDiffSentCount;
this.incompatibleClusterStateDiffVersionCount = incompatibleClusterStateDiffVersionCount;
}

public PublishClusterStateStats(StreamInput streamInput) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

We normally name these in, it leaves for shorter code and it would be consistent with other places in the codebase.

fullClusterStateSentCount = streamInput.readVLong();
clusterStateDiffSentCount = streamInput.readVLong();
incompatibleClusterStateDiffVersionCount = streamInput.readVLong();
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeVLong(fullClusterStateSentCount);
out.writeVLong(clusterStateDiffSentCount);
out.writeVLong(incompatibleClusterStateDiffVersionCount);
}

static final class Fields {
Copy link
Member

Choose a reason for hiding this comment

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

We've been moving away from these inner fields classes, we don't need them to encapsulate some strings.

static final String PUBLISH_CLUSTER_STATE = "publish_cluster_state";
static final String FULL_SENT = "full_cluster_states_sent";
static final String DIFFS_SENT = "cluster_state_diffs_sent";
static final String INCOMPATIBLE_DIFFS = "incompatible_cluster_state_diffs_sent";
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(Fields.PUBLISH_CLUSTER_STATE);
builder.field(Fields.FULL_SENT, fullClusterStateSentCount);
builder.field(Fields.DIFFS_SENT, clusterStateDiffSentCount);
builder.field(Fields.INCOMPATIBLE_DIFFS, incompatibleClusterStateDiffVersionCount);
builder.endObject();
return builder;
Copy link
Member

Choose a reason for hiding this comment

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

Here you can do something like this:

diff --git a/core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java b/core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java
index 356b9a29dc..36794e880f 100644
--- a/core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java
+++ b/core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java
@@ -65,9 +65,11 @@ public class PublishClusterStateStats implements Writeable, ToXContentObject {
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject("published_cluster_states");
-        builder.field("full_states", fullClusterStateReceivedCount);
-        builder.field("incompatible_diffs", incompatibleClusterStateDiffReceivedCount);
-        builder.field("compatible_diffs", compatibleClusterStateDiffReceivedCount);
+        {
+            builder.field("full_states", fullClusterStateReceivedCount);
+            builder.field("incompatible_diffs", incompatibleClusterStateDiffReceivedCount);
+            builder.field("compatible_diffs", compatibleClusterStateDiffReceivedCount);
+        }
         builder.endObject();
         return builder;
     }

which makes the JSON-structure clearer in the code.

}

public long getFullClusterStateSentCount() {
return fullClusterStateSentCount;
}

public long getClusterStateDiffSentCount() {
return clusterStateDiffSentCount;
}

public long getIncompatibleClusterStateDiffVersionCount() {
return incompatibleClusterStateDiffVersionCount;
}

@Override
public String toString() {
return "PublishClusterStateStats(full=" + fullClusterStateSentCount
+ ", diffs=" + clusterStateDiffSentCount
+ ", incompatible=" + incompatibleClusterStateDiffVersionCount
+ ")";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,8 @@ Set<DiscoveryNode> getFaultDetectionNodes() {
@Override
public DiscoveryStats stats() {
PendingClusterStateStats queueStats = pendingStatesQueue.stats();
return new DiscoveryStats(queueStats);
PublishClusterStateStats publishStats = publishClusterState.stats();
return new DiscoveryStats(queueStats, publishStats);
Copy link
Member

Choose a reason for hiding this comment

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

I think:

diff --git a/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java b/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java
index 5f62ee6b97..d688a5d5cd 100644
--- a/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java
+++ b/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java
@@ -412,9 +412,7 @@ public class ZenDiscovery extends AbstractLifecycleComponent implements Discover
 
     @Override
     public DiscoveryStats stats() {
-        PendingClusterStateStats queueStats = pendingStatesQueue.stats();
-        PublishClusterStateStats publishStats = publishClusterState.stats();
-        return new DiscoveryStats(queueStats, publishStats);
+        return new DiscoveryStats(pendingStatesQueue.stats(), publishClusterState.stats());
     }
 
     public DiscoverySettings getDiscoverySettings() {

is fine?

}

public DiscoverySettings getDiscoverySettings() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.discovery.DiscoveryStats;
import org.elasticsearch.discovery.zen.PendingClusterStateStats;
import org.elasticsearch.discovery.zen.PublishClusterStateStats;
import org.elasticsearch.http.HttpStats;
import org.elasticsearch.indices.breaker.AllCircuitBreakerStats;
import org.elasticsearch.indices.breaker.CircuitBreakerStats;
Expand Down Expand Up @@ -392,8 +393,16 @@ private static NodeStats createNodeStats() {
}
ScriptStats scriptStats = frequently() ?
new ScriptStats(randomNonNegativeLong(), randomNonNegativeLong(), randomNonNegativeLong()) : null;
DiscoveryStats discoveryStats = frequently() ? new DiscoveryStats(randomBoolean() ? new PendingClusterStateStats(randomInt(),
randomInt(), randomInt()) : null) : null;
DiscoveryStats discoveryStats = frequently()
? new DiscoveryStats(
randomBoolean()
? new PendingClusterStateStats(randomInt(), randomInt(), randomInt())
: null,
randomBoolean()
? new PublishClusterStateStats(randomNonNegativeLong(),
randomNonNegativeLong(), randomNonNegativeLong())
: null)
: null;
IngestStats ingestStats = null;
if (frequently()) {
IngestStats.Stats totalStats = new IngestStats.Stats(randomNonNegativeLong(), randomNonNegativeLong(), randomNonNegativeLong(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,11 @@ public void testDiscoveryStats() throws Exception {
" \"total\" : 0,\n" +
" \"pending\" : 0,\n" +
" \"committed\" : 0\n" +
" },\n" +
" \"publish_cluster_state\" : {\n" +
" \"full_cluster_states_sent\" : 0,\n" +
" \"cluster_state_diffs_sent\" : 0,\n" +
" \"incompatible_cluster_state_diffs_sent\" : 0\n" +
" }\n" +
" }\n" +
"}";
Expand All @@ -275,6 +280,11 @@ public void testDiscoveryStats() throws Exception {
assertThat(stats.getQueueStats().getCommitted(), equalTo(0));
assertThat(stats.getQueueStats().getPending(), equalTo(0));

assertThat(stats.getPublishStats(), notNullValue());
assertThat(stats.getPublishStats().getFullClusterStateSentCount(), equalTo(0L));
assertThat(stats.getPublishStats().getClusterStateDiffSentCount(), equalTo(0L));
assertThat(stats.getPublishStats().getIncompatibleClusterStateDiffVersionCount(), equalTo(0L));

XContentBuilder builder = XContentFactory.jsonBuilder().prettyPrint();
builder.startObject();
stats.toXContent(builder, ToXContent.EMPTY_PARAMS);
Expand Down