ClusterStateTaskListener usage refactoring in MasterServiceTests #82869

idegtiarenko · 2022-01-20T15:57:54Z

Today node removal tasks executed by the master have a separate
ClusterStateTaskListener to feed back the result to the requester.
It'd be preferable to use the task itself as the listener.

Rel: #82644

Today node removal tasks executed by the master have a separate ClusterStateTaskListener to feed back the result to the requester. It'd be preferable to use the task itself as the listener.

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java

elasticmachine · 2022-01-20T17:17:04Z

Pinging @elastic/es-distributed (Team:Distributed)

DaveCTurner

Looks good, I left some comments.

DaveCTurner · 2022-01-20T17:31:19Z

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java

-        int numberOfExecutors = Math.max(1, numberOfThreads / 4);
+        final int numberOfThreads = randomIntBetween(2, 8);
+        final int taskSubmissionsPerThread = randomIntBetween(1, 64);
+        final int numberOfExecutors = Math.max(1, numberOfThreads / 4);
        final Semaphore semaphore = new Semaphore(numberOfExecutors);


This semaphore seems unnecessary to me, maybe I'm missing something?

If ClusterStateTaskListener#clusterStateProcessed is called after the ClusterStateTaskExecutor#clusterStatePublished then we can guarantee all tasks are completed just with processedStatesLatch. Let me confirm this assumption

I was missing something: I didn't see the final call to Semaphore#acquire 🤦

It looks like the order of execution is:

ClusterStateTaskExecutor#execute

N * ClusterStateTaskListener#clusterStateProcessed

ClusterStateTaskExecutor#clusterStatePublished(ClusterStatePublicationEvent clusterPublicationEvent)

This guards us from failing following assertion: assertEquals(executor.batches.get(), executor.published.get());

As discussed in another channel, there's only one thread here so this can be a new Semaphore(1) instead, and the semaphore.acquire() call that happens when the task is executed should be assertTrue(semaphore.tryAcquire()) instead.

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java

DaveCTurner · 2022-01-20T17:34:29Z

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java

@@ -464,23 +464,39 @@ public void onFailure(Exception e) {
    }

    public void testClusterStateBatchedUpdates() throws BrokenBarrierException, InterruptedException {


One important thing that this test seems not to check is that batches submitted with the same executor are executed together. It's a bit tricky to test because you have to block the master service while you're submitting the batches to make sure that they all arrive "at once". Still, I think this is something worth addressing either randomly in this test or else in a separate test.

On reflection I think it doesn't make sense to try and extend this test to cover this extra invariant too. Let's leave it as it is and add a new test in a follow-up.

I believe that is asserted here: https://github.com/idegtiarenko/elasticsearch/blob/665c374b90098f4dd34bcf238de21342a2b4c47a/server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java#L538-L545 (unless I miss something)

UPD: it is not checking "all at once" but at least it is verifying the grouping

This asserts that every set of tasks that was submitted together is executed as an atomic unit. That's certainly important but in practice we only submit multiple tasks in CandidateJoinAccumulator when completing an election. We also want to know that tasks submitted with multiple calls to submitStateUpdateTask are executed as a batch.

idegtiarenko · 2022-01-24T09:28:18Z

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java


-            TaskExecutor(List<Set<Task>> taskGroups) {
-                this.taskGroups = taskGroups;
-            }


Moved this to assignments. Now executor will verify it only executes own tasks

I think we don't actually verify this yet, we're now only checking that every group of our own tasks is completely executed. Previously we were checking that every group of any executor's tasks is completely executed so technically this is now a weaker test.

I think this would be a good property to verify, but to do this we also need to say that every task we're executing belongs to one of our own groups.

I just added assertThat("All tasks should belong to this executor", totalCount, equalTo(tasks.size())); to verify that every single task from the list belongs to the current executor.

There is also the a check in the very end of the test that assertEquals(executor.assigned.get(), executor.executed.get()) that verifies that all assigned tasks are executed. We also have a check that no task is executed twice.

I believe this should cover everything, is it?

idegtiarenko · 2022-01-24T10:12:57Z

@elasticmachine please run elasticsearch-ci/part-1

DaveCTurner

Looks good, I left a few cosmetic suggestions and one request.

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java

DaveCTurner · 2022-01-24T11:34:26Z

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java


-            TaskExecutor(List<Set<Task>> taskGroups) {
-                this.taskGroups = taskGroups;
-            }


I think we don't actually verify this yet, we're now only checking that every group of our own tasks is completely executed. Previously we were checking that every group of any executor's tasks is completely executed so technically this is now a weaker test.

I think this would be a good property to verify, but to do this we also need to say that every task we're executing belongs to one of our own groups.

DaveCTurner

LGTM

DaveCTurner · 2022-01-24T13:20:16Z

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java

                }
+                assertThat("All tasks should belong to this executor", totalCount, equalTo(tasks.size()));


idegtiarenko added 2 commits January 20, 2022 15:53

MasterServiceTests ClusterStateTaskListener usage refactoring

bc36cb4

Today node removal tasks executed by the master have a separate ClusterStateTaskListener to feed back the result to the requester. It'd be preferable to use the task itself as the listener.

update test

0e9ca67

idegtiarenko requested a review from DaveCTurner January 20, 2022 15:57

idegtiarenko mentioned this pull request Jan 20, 2022

Make every master task a ClusterStateTaskListener #82644

Closed

9 tasks

idegtiarenko commented Jan 20, 2022

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java Show resolved Hide resolved

idegtiarenko added 2 commits January 20, 2022 17:03

simplify

8c95254

cleanup

665c374

idegtiarenko marked this pull request as ready for review January 20, 2022 17:17

DaveCTurner reviewed Jan 20, 2022

View reviewed changes

idegtiarenko added 3 commits January 24, 2022 09:25

keep assignments with task executor

afe6edb

assert all modifications happen on a single thread

8c6898f

Merge branch 'master' into 82644_master_service_tests

0387a0b

idegtiarenko requested a review from DaveCTurner January 24, 2022 09:04

idegtiarenko commented Jan 24, 2022

View reviewed changes

prevent empty task assignments

6fcf1ce

DaveCTurner reviewed Jan 24, 2022

View reviewed changes

idegtiarenko added 2 commits January 24, 2022 13:17

add extra check

0f4a44b

Merge branch 'master' into 82644_master_service_tests

bc02a84

idegtiarenko requested a review from DaveCTurner January 24, 2022 12:52

DaveCTurner approved these changes Jan 24, 2022

View reviewed changes

idegtiarenko merged commit 805cd39 into elastic:master Jan 24, 2022

idegtiarenko deleted the 82644_master_service_tests branch January 24, 2022 13:30

		@@ -464,23 +464,39 @@ public void onFailure(Exception e) {
		}

		public void testClusterStateBatchedUpdates() throws BrokenBarrierException, InterruptedException {

		}
		assertThat("All tasks should belong to this executor", totalCount, equalTo(tasks.size()));

ClusterStateTaskListener usage refactoring in MasterServiceTests #82869

ClusterStateTaskListener usage refactoring in MasterServiceTests #82869

Uh oh!

Conversation

idegtiarenko commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Jan 20, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idegtiarenko Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idegtiarenko commented Jan 24, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

idegtiarenko commented Jan 20, 2022 •

edited

Loading

idegtiarenko Jan 24, 2022 •

edited

Loading