[Zen2] Implement Tombstone REST APIs #36007

original-brownbear · 2018-11-28T15:31:24Z

Adds REST API for withdrawing votes and clearing vote withdrawls
Tests added to Netty4 module since we need a real Network impl. for Http endpoints

* Adds REST API for withdrawing votes and clearing vote withdrawls * Tests added to Netty4 module since we need a real Network impl. for Http endpoints

elasticmachine · 2018-11-28T15:31:26Z

Pinging @elastic/es-distributed

DaveCTurner

A few questions and nits.

DaveCTurner · 2018-11-28T17:40:57Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

+    protected Settings nodeSettings(int nodeOrdinal) {
+        return Settings.builder().put(super.nodeSettings(nodeOrdinal))
+            .put(TestZenDiscovery.USE_ZEN2.getKey(), true)
+            .put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), 2)


Was this necessary?

Yea, the internal test cluster throws if this isn't set and you turn off auto manage min master nodes (probably something that could be adjusted for Zen2?).

Oh yes, so it does. Set it to something unreasonable (MAX_VALUE) to protect against this test passing without Zen2.

DaveCTurner · 2018-11-28T17:41:23Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

+            .put(TestZenDiscovery.USE_ZEN2.getKey(), true)
+            .put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), 2)
+            .put(ClusterBootstrapService.INITIAL_MASTER_NODE_COUNT_SETTING.getKey(), 2)
+            .put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), "5s")


This shouldn't be necessary, the cluster should form straight away.

DaveCTurner · 2018-11-28T17:41:46Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

+    }
+
+    public void testAddAndClearVotingTombstones() throws Exception {
+        final int nodeCount = 2;


Could you inline nodeCount?

DaveCTurner · 2018-11-28T17:42:24Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

+        return false; // enable http
+    }
+
+    public void testAddAndClearVotingTombstones() throws Exception {


I think testRollingRestartOfTwoNodeCluster would be a good name.

DaveCTurner · 2018-11-28T17:43:16Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

+                        .setWaitForNodes(Integer.toString(nodeCount - 1))
+                        .setTimeout(TimeValue.timeValueSeconds(30L));
+
+                    clusterHealthRequestBuilder.setWaitForYellowStatus();


This can be in the chain of .set() methods above, and the temporary clusterHealthRequestBuilder can probably be inlined.

DaveCTurner · 2018-11-28T17:43:35Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

+        Response deleteResponse = restClient.performRequest(new Request("DELETE", "/_cluster/withdrawn_votes"));
+        assertThat(deleteResponse.getStatusLine().getStatusCode(), is(200));
+        assertThat(deleteResponse.getEntity().getContentLength(), is(0L));
+        Response response =


Why the
newline? :)

DaveCTurner · 2018-11-28T17:44:39Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

+    public void testBasicRestApi() throws Exception {
+        List<String> nodes = internalCluster().startNodes(3);
+        RestClient restClient = getRestClient();
+        Response deleteResponse = restClient.performRequest(new Request("DELETE", "/_cluster/withdrawn_votes"));


Think it'd make more sense to put this after the POST. There will, at some point, be an assertion that there's no voting tombstones in the cluster at the end of the test.

I agree. Just tried making that change but running into this error as a result:

[2018-11-29T00:04:21,996][WARN ][r.suppressed ] [node_t0] path: /_cluster/withdrawn_votes, params: {} org.elasticsearch.transport.RemoteTransportException: [node_t1][127.0.0.1:33009][cluster:admin/voting/clear_tombstones] Caused by: org.elasticsearch.ElasticsearchTimeoutException: timed out waiting for removal of nodes; if nodes should not be removed, set waitForRemoval to false. [{node_t2}{ps-Qi3TfSAOMEW6D-A5uAA}] at org.elasticsearch.action.admin.cluster.configuration.TransportClearVotingTombstonesAction$1.onTimeout(TransportClearVotingTombstonesAction.java:109) ~[main/:?] at org.elasticsearch.cluster.ClusterStateObserver$ContextPreservingListener.onTimeout(ClusterStateObserver.java:322) ~[main/:?] at org.elasticsearch.cluster.ClusterStateObserver$ObserverClusterStateListener.onTimeout(ClusterStateObserver.java:249) ~[main/:?] at org.elasticsearch.cluster.service.ClusterApplierService$NotifyTimeout.run(ClusterApplierService.java:561) ~[main/:?] at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:627) ~[main/:?] at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) ~[?:?] at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) ~[?:?] at java.lang.Thread.run(Thread.java:834) [?:?]

(no matter what node I add a Tombstone for, this happens)

if nodes should not be removed, waitForRemoval to false

Should I do that in the Rest API?

original-brownbear · 2018-11-28T18:07:24Z

All but 2 comments addressed :) 2 Questions added.

original-brownbear · 2018-11-29T05:30:14Z

@DaveCTurner alright added the "don't wait" parameter and set the zen1 master count to Integer.MAX_VALUE => should be good for review again :)

DaveCTurner

My review was unclear - I intended exposing the parameter, not hard-coding its value.

DaveCTurner · 2018-11-29T08:45:56Z

...c/main/java/org/elasticsearch/rest/action/admin/cluster/RestClearVotingTombstonesAction.java

+    @Override
+    protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
+        ClearVotingTombstonesRequest req = new ClearVotingTombstonesRequest();
+        req.setWaitForRemoval(false);


Apologies, I meant this should be a parameter ?waitForRemoval=false - both options are useful in different circumstances. The default of true is the usual case, but in this particular test we should set it to false because the node is still present.

Now that I've written that, I think it'd be good to test both cases:

create a 3-node cluster, add a tombstone, then clear them (?waitForRemoval=false) <- today's test

create a 3-node cluster, add a tombstone, shut the corresponding node down, then clear the tombstones (?waitForRemoval=true)

Looking at other APIs, it should be wait_for_removal not waitForRemoval.

DaveCTurner · 2018-11-29T08:52:51Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

+
+import static org.hamcrest.core.Is.is;
+
+// TODO: Move these tests to a more appropriate module


Suggest:

// These tests are here today so they have access to a proper REST client. They cannot be in :server:integTest since the REST client needs a // proper transport implementation, and they cannot be REST tests today since they need to restart nodes. When #35599 and friends land we // should be able to move these tests to run against a proper cluster instead. TODO do this.

original-brownbear · 2018-11-29T09:29:00Z

@DaveCTurner all done :)

exposed wait param
- Added tests for both cases
reworded todo

DaveCTurner

LGTM

original-brownbear · 2018-11-29T12:14:22Z

Jenkins test this

[Zen2] Implement Tombstone REST APIs

6cae1b8

* Adds REST API for withdrawing votes and clearing vote withdrawls * Tests added to Netty4 module since we need a real Network impl. for Http endpoints

original-brownbear added >enhancement v7.0.0 :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Nov 28, 2018

original-brownbear requested a review from DaveCTurner November 28, 2018 15:32

DaveCTurner reviewed Nov 28, 2018

View reviewed changes

ywelsch mentioned this pull request Nov 28, 2018

A new cluster coordination layer #32006

Closed

61 tasks

CR: comments

e167a09

CR: don't wait for node removal + safer setting for zen1 node count

8456401

DaveCTurner reviewed Nov 29, 2018

View reviewed changes

CR: expose param

0d5d5a7

DaveCTurner approved these changes Nov 29, 2018

View reviewed changes

Merge remote-tracking branch 'elastic/zen2' into zen2-rest-apis

2305b50

original-brownbear merged commit 48dc6c3 into elastic:zen2 Nov 29, 2018

original-brownbear deleted the zen2-rest-apis branch November 29, 2018 13:34

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

[Zen2] Implement Tombstone REST APIs #36007

[Zen2] Implement Tombstone REST APIs #36007

original-brownbear commented Nov 28, 2018

elasticmachine commented Nov 28, 2018

DaveCTurner left a comment

DaveCTurner Nov 28, 2018

original-brownbear Nov 28, 2018

DaveCTurner Nov 28, 2018

DaveCTurner Nov 28, 2018

DaveCTurner Nov 28, 2018

DaveCTurner Nov 28, 2018

DaveCTurner Nov 28, 2018

DaveCTurner Nov 28, 2018

DaveCTurner Nov 28, 2018

original-brownbear Nov 28, 2018

DaveCTurner Nov 28, 2018

original-brownbear commented Nov 28, 2018

original-brownbear commented Nov 29, 2018

DaveCTurner left a comment

DaveCTurner Nov 29, 2018

DaveCTurner Nov 29, 2018

DaveCTurner Nov 29, 2018

original-brownbear commented Nov 29, 2018

DaveCTurner left a comment

original-brownbear commented Nov 29, 2018


		import static org.hamcrest.core.Is.is;

		// TODO: Move these tests to a more appropriate module

[Zen2] Implement Tombstone REST APIs #36007

[Zen2] Implement Tombstone REST APIs #36007

Conversation

original-brownbear commented Nov 28, 2018

elasticmachine commented Nov 28, 2018

DaveCTurner 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Nov 28, 2018

original-brownbear commented Nov 29, 2018

DaveCTurner 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

original-brownbear commented Nov 29, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

original-brownbear commented Nov 29, 2018