Skip to content

'shutdownAllTasks' API for a dataSource#6185

Merged
gianm merged 3 commits intoapache:masterfrom
QiuMM:shutdown
Aug 17, 2018
Merged

'shutdownAllTasks' API for a dataSource#6185
gianm merged 3 commits intoapache:masterfrom
QiuMM:shutdown

Conversation

@QiuMM
Copy link
Copy Markdown
Member

@QiuMM QiuMM commented Aug 16, 2018

Try to fix #6115

Change-Id: I30d14390457d39e0427d23a48f4f224223dc5777

Shuts down a task.

* `druid/indexer/v1/task/{dataSource}/shutdown`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This namespace collides with {taskId}. It's not likely that anyone will actually create a task with the same name as a datasource, but you never know, so let's use a different URL here. How about: /druid/indexer/v1/dataSource/{dataSource}/shutdownAllTasks

}

if (!ownTask) {
return Response.status(Response.Status.BAD_REQUEST)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we should return OK in this case, because that makes the kill-all operation idempotent. It's a good property to have, especially in something that might be done "just in case" when removing a datasource.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, I'll remove the ownTask variable and return Response.ok(ImmutableMap.of("dataSource", dataSource)).build() for all case. Is that OK?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me… thanks!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix!

Change-Id: Ib463f31ee2c4cb168cf2697f149be845b57c42e5
taskQueue.shutdown(task.getId());
ownTask = true;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use this API getActiveTaskInfo(@Nullable String dataSource) which takes a dataSource as argument and filters at the DB query time, so you will only get the matching dataSource active tasks. So like below. It might be efficient as you don't have to compare every task's dataSource in the for loop.

final List<TaskInfo<Task, TaskStatus>> tasks = taskStorageQueryAdapter.getActiveTaskInfo(dataSource);
            for (final TaskInfo<Task, TaskStatus> task : tasks) {
              taskQueue.shutdown(task.getId());
            }``` 

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, fixed!

Change-Id: I50a8dcd44dd9d36c9ecbfa78e103eb9bff32eab9
public Response apply(TaskQueue taskQueue)
{
final List<TaskInfo<Task, TaskStatus>> tasks = taskStorageQueryAdapter.getActiveTaskInfo(dataSource);
for (final TaskInfo<Task, TaskStatus> task : tasks) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe tasks.parallelStream().forEach(t -> taskQueue.shutdown(t.getId())) will be better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think so, taskQueue.shutdown is a synchronized operation. Use parallelStream has no performance improvement.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. Even if ReentrantLock is good at handling with concurrent scenes, we should not try to use parallelStream here. I didn't notice it. My fault. Forget it, what do you think of converting it into a more functional programming style, such as tasks.forEach(t -> taskQueue.shutdown(t.getId()))?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no need. Other for loop code in class OverlordResource are not functional programming style. Coding style should keep consistency.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it's up to you. 😅

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that it's good to be consistent within a file, and the rest of this file is totally down with for loops.

@QiuMM
Copy link
Copy Markdown
Member Author

QiuMM commented Aug 17, 2018

@gianm Can you help restart the failed travis job. Thanks.

@nishantmonu51
Copy link
Copy Markdown
Member

have restarted the failing travis.

@gianm gianm merged commit b0cf8d0 into apache:master Aug 17, 2018
@QiuMM QiuMM deleted the shutdown branch August 24, 2018 08:00
@dclim dclim added this to the 0.13.0 milestone Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Druid should have a 'shutdownAllTasks' API for a dataSource

6 participants