Make the Task object available to the action caller #16033
Conversation
imotov
added
>feature
review
:Distributed Coordination/Task Management
| CountDownLatch latch = new CountDownLatch(1); | ||
| AtomicReference<ListTasksResponse> response = new AtomicReference<>(); | ||
| final Client client; | ||
| boolean useDataClient = randomBoolean(); |
There was a problem hiding this comment.
if useDataClient is false you still might end up with a data client, right? its more like forceDataNodeClient?
|
Makes sense to me. I hadn't thought about it not being available to the transport client but it makes sense and should work fine for my purpose. I'd copy that javadoc about it returning null to ActionRequestBuilder as well. |
|
@nik9000 I thought more about it. Do we actually need to expose it all the way up to the client level for your use case? In the scenario where the task is actually available in this PR (the caller is calling execute on the node client) the caller typically has direct access to If there is a real need to expose the task information all the way to the client level then there should be no difference between a node client and a transport client and we need to devise some method to ship the started task id back to the caller, which is much more complicated. Since the task might be executed on a remote node, we cannot return the So, if What do you think? |
|
Right. I see your way of thinking and I think I agree. If it can't work for TransportClient I don't think it should be part of the interface. While I'd love for clients to have access to task information - TaskInfo would be wonderful, but I had a quick look and I don't see any kind of "I've started your request" message that we could piggy back on so I don't think its worth it doing. TransportAction level access will work I think. It'll make something like #15687 more complicated but that is life. |
|
So if you amend the PR to drop the Client level stuff and just do TransportAction level stuff you still have my LGTM. |
|
Even better, it looks like without Client level changes this PR is about 2 lines. |
|
Not sure I follow the conversation above, but I'd imagine doing a reindex request with |
Yup. That is what it'll do regardless of what we do above. This is more a question of "where do tasks show up in the API" and "what do rest tasks have to do to return the task ID rather than wait for completion" and "does the java client get to use them easily." The answers are "not in the client API. not yet anyway" and "get one or two more dependencies injected than normal and use them in obvious ways" and "no". |
imotov
force-pushed
the
expose-task-to-the-caller
branch
from
2dc6a88 to
89901a0
Compare
|
@nik9000 I pushed the new version. I think I like it much more :) |
|
LGTM. Its much simpler. If we want to give the clients some kind of task object on every request we'll do it as a separate PR. |
Sometimes action callers might be interested in having an access to the task that they have just initiated. This changes allows callers to get access to the Task object of the actions that they just started if the action supports the task management.
imotov
force-pushed
the
expose-task-to-the-caller
branch
from
89901a0 to
d9a0122
Compare
| @@ -66,7 +66,7 @@ protected TransportAction(Settings settings, String actionName, ThreadPool threa | |||
| return future; | |||
| } | |||
|
|
|||
| public final void execute(Request request, ActionListener<Response> listener) { | |||
| public final Task execute(Request request, ActionListener<Response> listener) { | |||
| Task task = taskManager.register("transport", actionName, request); | |||
| if (task == null) { | |||
There was a problem hiding this comment.
when do we expect task to be null?
There was a problem hiding this comment.
@bleskes I added an option for an action to opt out from task management tracking. If the TransportRequest#createTask method returns null instead of a task object, this action is not tracked by the task manager and the register call returns null.
I forgot to document this behavior. Thanks for the reminder. I will add comments in a bit.
Sometimes action callers might be interested in having access to the task that they have just initiated. This changes allows a caller to get access to the Task object of an action that it just started if the action runs on the same node and supports the task management.
@nik9000 do you want to review it?