[Part 7]: Introducing the Task Executor #157

sundargates · 2022-03-29T20:52:34Z

Context

So far, we have seen several classes on the worker side that are meant to help with the on-demand task execution. This diff introduces the task executor class that binds all these classes to implement the gateway.

Checklist

./gradlew build compiles code correctly
Added new tests where applicable
./gradlew test passes all tests
Extended README or added javadocs where applicable
Added copyright headers for new files from CONTRIBUTING.md

So far, we have seen several classes on the worker side that are meant to help with the on-demand task execution. This diff introduces the task executor class that binds all these classes to implement the gateway.

calvin681 · 2022-03-29T22:26:09Z

...lane-client/src/main/java/io/mantisrx/server/master/client/HighAvailabilityServicesUtil.java

+                        .newSingleThreadExecutor(
+                            new ThreadFactoryBuilder().setNameFormat(nameFormat).build()));
+
+        final List<Subscription> subscriptions = new ArrayList<>();


Where do we use these subscriptions?

Nowhere right now. But, I think the intention when I wrote the code (a long time ago) was that we would close these subscriptions when the task executor is about to be shut down. So, it just makes for a clean, graceful shutdown.

codyrioux

I love the testing this enables visible in TaskExecutorTest.java. Great work. Approving with the condition of getting the CI check fixed.

codyrioux · 2022-03-30T17:15:31Z

...nt/src/main/java/io/mantisrx/server/master/resourcecluster/ResourceClusterGatewayClient.java

+    return performAction("disconnectTaskExecutor", taskExecutorDisconnection);
+  }
+
+  private CompletableFuture<Ack> performAction(String action, Object body) {


nit: An enum or similar closed form solution might be safer than a free form string for the action parameter.

hmm, this is strictly within the class - so I'm not sure what value enum would particularly add. If this was exposed, I agree enums are definitely better as they are stronger types than free-form strings.

calvin681 · 2022-03-30T18:27:21Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

+    }
+
+    private void startTaskExecutorServices() throws Exception {
+        validateRunsInMainThread();


why do we need to run in main thread?

We want all state changes on the main thread similar to an actor. Any IO actions can run on the IO executor.

...ol-plane-client/src/main/java/io/mantisrx/server/master/client/HighAvailabilityServices.java

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

Andyz26 · 2022-03-31T17:55:09Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

+                } catch (Exception inner) {
+                    log.error("Disconnection has also failed", inner);
+                }
+                throw e;


Is the handling policy here to crash and restart the whole process on each request failure? Shall we consider some retries?

No, the process does not restart. When the resource manager gateway connection fails to establish, the service goes from the "RUNNING" to the "FAILED" state. The task executor at this point (which is listening to these state transitions) would try to reestablish a new connection with the resource manager. So, the process doesn't restart.

Andyz26 · 2022-03-31T18:00:53Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

+        }
+
+        @Override
+        public void runOneIteration() throws Exception {


The method name doesn't seem to match the work inside (sending heartbeat report).

This is actually a method of AbstractScheduledService. It's just a nice framework to write these services.

Andyz26 · 2022-03-31T18:10:23Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/JvmUtils.java

+ * limitations under the License.
+ */
+
+package io.mantisrx.server.worker;


nit: this seems generic enough for both worker/server.

Hmm, we don't have a usage for this in the server right now. Will move it at a later point when there's a concrete usage.

Andyz26 · 2022-03-31T18:12:37Z

...ver/mantis-server-worker/src/main/java/io/mantisrx/server/worker/MachineDefinitionUtils.java

+import io.mantisrx.runtime.MachineDefinition;
+
+public class MachineDefinitionUtils {
+  public static MachineDefinition sys(WorkerPorts workerPorts) {


"WithWorkerPorts"?

Andyz26 · 2022-03-31T18:14:35Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

+
+    public TaskExecutor(RpcService rpcService,
+                        WorkerConfiguration workerConfiguration,
+                        HighAvailabilityServices highAvailabilityServices, ClassLoaderHandle classLoaderHandle,


nit: inconsistent format on line breaks

Good point. Cleaned it up.

Andyz26 · 2022-03-31T18:14:37Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

+
+    public TaskExecutor(RpcService rpcService,
+                        WorkerConfiguration workerConfiguration,
+                        HighAvailabilityServices highAvailabilityServices, ClassLoaderHandle classLoaderHandle,


nit: inconsistent format on line breaks

Andyz26 · 2022-03-31T18:17:19Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

+            .register(new ResourceManagerChangeListener());
+    }
+
+    public CompletableFuture<Void> awaitRunning() {


Is this used? Cannot find any ref to it.

This is used in the tests. In the tests, it's important to know when the task executor has started so that we can start issuing commands to it.

sundargates · 2022-03-31T17:45:52Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

+        validateRunsInMainThread();
+
+        masterMonitor = highAvailabilityServices.getMasterClientApi();
+        RxNetty.useMetricListenersFactory(new MantisNettyEventsListenerFactory());


why are we doing this?

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

sundargates · 2022-03-31T17:49:16Z

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

+    private ResourceManagerGatewayCxn currentResourceManagerCxn;
+    private TaskExecutorReport currentReport;
+    private Task currentTask;
+    private Subscription currentTaskStatusSubscription;


why is this being tracked?

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java

sundargates added 2 commits March 29, 2022 13:05

[Part 7]: Introducing the task executor

5aacd70

So far, we have seen several classes on the worker side that are meant to help with the on-demand task execution. This diff introduces the task executor class that binds all these classes to implement the gateway.

Adding unit and integration tests

de625a6

sundargates requested review from codyrioux, nickmahilani, jeffchao, piygoyal, calvin681, Andyz26, hmitnflx, markcho and liuml07 as code owners March 29, 2022 20:52

calvin681 reviewed Mar 29, 2022

View reviewed changes

Fixing test

ebc355c

calvin681 approved these changes Mar 30, 2022

View reviewed changes

codyrioux approved these changes Mar 30, 2022

View reviewed changes

calvin681 reviewed Mar 30, 2022

View reviewed changes

Ignoring the integration test that can only be run locally

ad6308f

Andyz26 reviewed Mar 31, 2022

View reviewed changes

...ol-plane-client/src/main/java/io/mantisrx/server/master/client/HighAvailabilityServices.java Show resolved Hide resolved

Andyz26 reviewed Mar 31, 2022

View reviewed changes

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/TaskExecutor.java Outdated Show resolved Hide resolved

Andyz26 reviewed Mar 31, 2022

View reviewed changes

sundargates commented Apr 1, 2022

View reviewed changes

sundargates added 3 commits April 1, 2022 11:52

Addressing comments

750cddc

Cleaning up

11b8705

Further cleaning up

2aeeb1c

sundargates added 2 commits April 1, 2022 13:12

Cleaning up TaskExecutorTest

46fd281

Cleaning up logs

fbec8bb

sundargates merged commit e78e7b2 into Netflix:master Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Part 7]: Introducing the Task Executor #157

[Part 7]: Introducing the Task Executor #157

sundargates commented Mar 29, 2022

calvin681 Mar 29, 2022

sundargates Mar 29, 2022

codyrioux left a comment •

edited

Loading

codyrioux Mar 30, 2022

sundargates Mar 31, 2022

calvin681 Mar 30, 2022

sundargates Mar 31, 2022

Andyz26 Mar 31, 2022

sundargates Apr 1, 2022

Andyz26 Mar 31, 2022

sundargates Apr 1, 2022

Andyz26 Mar 31, 2022

sundargates Apr 1, 2022

Andyz26 Mar 31, 2022

Andyz26 Mar 31, 2022

sundargates Apr 1, 2022

Andyz26 Mar 31, 2022

Andyz26 Mar 31, 2022

sundargates Apr 1, 2022

sundargates Mar 31, 2022

sundargates Mar 31, 2022

[Part 7]: Introducing the Task Executor #157

[Part 7]: Introducing the Task Executor #157

Conversation

sundargates commented Mar 29, 2022

Context

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrioux left a comment • edited Loading

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

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

codyrioux left a comment •

edited

Loading