Task Management: Add framework for registering and communicating with tasks #15347
Conversation
imotov
added
>feature
review
:Distributed/Task Management
| * | ||
| * The class is final due to serialization limitations | ||
| */ | ||
| public final class TaskOperationFailedException implements Streamable, ToXContent { |
There was a problem hiding this comment.
it super weird that this class doesn't subclass Exception - I think it should be TaskOperationFailure instead?
There was a problem hiding this comment.
we should also implement Writeable instead to make it immutable?
|
@imotov I left a bunch of comments and some requests on how to make the change way smaller and way less intrusive. I want those to be addressed before reviewing more of this. I don't think we can go with adding Task and friends everywhere this is just not the way we can expose a feature like this. I am happy to discuss the alternatives but I gave you examples in my comments |
imotov
force-pushed
the
issue-6914-task-management
branch
from
f87f36d
to
f26f4d0
Compare
|
@s1monw I pushed the changes that you have requests. Could you take another look when you have a chance? |
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.field("getTaskId", getTaskId()); |
There was a problem hiding this comment.
can we use task_id instead?
| /** | ||
| * Reads a list of objects | ||
| */ | ||
| public <T> List<T> readList(Reader<T> producer) throws IOException { |
There was a problem hiding this comment.
do we need this Reader interface? can't this just be Funciton<StreamInput, T> reader
There was a problem hiding this comment.
Exactly, Function<StreamInput, T> reader cannot throw any checked exceptions and all our reading methods throw IOException.
There was a problem hiding this comment.
ok makes sense - we already have IOSupplier for the same reason, I think we should maybe add them as seperate classes in a util package?
|
I left some minor comments - LGTM otherwise |
| import org.elasticsearch.threadpool.ThreadPool; | ||
| import org.elasticsearch.transport.TransportService; |
|
Left minor comments. LGTM as well. I'm excited to get this merged and start integrating with it! |
imotov
force-pushed
the
issue-6914-task-management
branch
from
567aa0a
to
6a947e0
Compare
… tasks Adds task manager class and enables all activities to register with the task manager. Currently, the immutable Transport*Activity class represents activity itself shared across all requests. This PR adds and an additional structure Task that keeps track of currently running requests and can be used to communicate with these requests using TransportTaskAction. Related to elastic#15117
imotov
force-pushed
the
issue-6914-task-management
branch
from
4ac65f9
to
a89dba2
Compare
Adds task manager class and enables all activities to register with the task manager. Currently, the immutable
Transport*Activityclass represents activity itself shared across all requests. This PR adds and an additional structure Task that keeps track of currently running requests and can be used to communicate with these requests usingTransportTaskAction.Related to #15117