[Part 1]: Support for hot task executors in Mantis #145
Conversation
As we are approaching adding support for Mantis on Titus, one of the first goals is to be able to support task executors that are fully loaded (hot) and capable of executing Mantis stages on demand. This diff is the first step in that direction. This change introduces a new API on the server-side that establishes the protocol that the task executor needs to use to communicate with the server.
sundargates
requested review from
codyrioux,
nickmahilani,
jeffchao,
piygoyal,
calvin681 and
Andyz26
as code owners
| return defaultObjectMapper.readerFor(expectedType).readValue(json); | ||
| } | ||
|
|
||
| public String toJson(Object object) throws IOException { |
There was a problem hiding this comment.
nit: I'd call out the semantic difference between this and static toJson in comments.
Also, could we make this static?
There was a problem hiding this comment.
I have removed all the static methods.
| @RequiredArgsConstructor(access = AccessLevel.PRIVATE) | ||
| @Value | ||
| public class ClusterID { | ||
| String resourceID; |
There was a problem hiding this comment.
this should be private?
There was a problem hiding this comment.
value classes by default in Lombok treat the fields as "private final".
Here's the documentation: https://projectlombok.org/features/Value
| /** | ||
| * Datastructure representing the task executor that needs to be disconnected from the resource cluster. | ||
| */ | ||
| @Value |
There was a problem hiding this comment.
value annotation creates an "allargsconstructor".
| import io.mantisrx.common.JsonSerializer; | ||
| import org.junit.Test; | ||
|
|
||
| public class TestTaskExecutorRegistration { |
There was a problem hiding this comment.
nit: we should follow convention. call this TaskExecutorRegistrationTest. Move the heartbeat test to TaskExecutorHeartbeatTest.
There was a problem hiding this comment.
Renamed the class.
| public class ClusterID { | ||
| String resourceID; | ||
|
|
||
| public static ClusterID of(String clusterIdStr) { |
There was a problem hiding this comment.
Just wondering what's your thought on static "of" v.s. lombok builder?
There was a problem hiding this comment.
I really love the way static factory methods named of read in client code. ClusterId.of("m5.2xl-1") reads so cleanly to a user even compared to the builder ClusterId.builder().name("m5.2xl-1").build(). However I think this can be achieved with Lombok using @RequiredArgsConstructor(staticName="of").
There was a problem hiding this comment.
Very true. "of" is definitely cleaner on the single arg ctor case.
There was a problem hiding this comment.
That is actually a good point, it can get confusing on the multi-arg constructor case. Especially if parameters are of the same type, which string represents what?
There was a problem hiding this comment.
Thanks for the suggestion. I have used the Lombok annotation in both the places where it makes sense.
| public interface ResourceClusterGateway { | ||
| /** | ||
| * triggered at the start of a task executor or when the task executor has lost connection to the resource cluster | ||
| * previously |
Context
As we are approaching adding support for Mantis on Titus, one of the first goals is to be able to support task executors that are fully loaded (hot) and capable of executing Mantis stages on demand. This diff is the first step in that direction. This change introduces a new API on the server-side that establishes the protocol that the task executor needs to use to communicate with the server.
Checklist
./gradlew buildcompiles code correctly./gradlew testpasses all testsCONTRIBUTING.md