[Rollup] Add `wait_for_completion` option to StopRollupJob API #34811

polyfractal · 2018-10-24T15:34:51Z

This adds a flag which allows the user to block the Stop API until the task has actually moved to a stopped state, instead of returning immediately.

If the flag is unset (default) the behavior is as before: returns immediately and stopping happens async in the background. If the flag is set to a TimeValue, it will block the call until the status changes to STOPPED (up to the specified time limit). If the time value is exceeded a timeout exception is thrown.

Question: The current code executes the Tasks operation on the SAME thread, which I'm assuming would be the networking thread. Since this opens the chance of blocking the thread temporarily, should we switch to MANAGEMENT or GENERIC or something?

Also, this is technically an enhancement but critical to a Kibana blocker, so I'd lobby that it be allowed into 6.5

Followup to #34574

/cc @jen-huang @cjcenizal

This adds a flag which allows the user to block the Stop API until the task has actually moved to a stopped state, instead of returning immediately. If the flag is unset (default) the behavior is as before: returns immediately. If the flag is set to a TimeValue, it will block the call until the status changes to `STOPPED` (up to the specified time limit). If the time value is exceeded a timeout exception is thrown. Note: The job will still likely move to STOPPED eventually, the timeout just means the blocking call timed out before the state changed.

elasticmachine · 2018-10-24T15:34:53Z

Pinging @elastic/es-search-aggs

polyfractal · 2018-10-24T16:30:52Z

Quick note: if we find this too risky to put in 6.5, #34574 should largely negate the race condition that Kibana was worried about. If this PR doesn't merge the behavior will just be:

user hits stop, spinny for a second, hits delete, gets an error about job still running, tries again (n times) until it finally deletes.

Not wonderful, but not end of the world either.

jimczi

Thanks @polyfractal . I left some comments.

jimczi · 2018-10-24T20:22:05Z

docs/reference/rollup/apis/stop-job.asciidoc

@@ -22,6 +22,14 @@ Stopping an already stopped job has no action.
 `job_id` (required)::
  (string) Identifier for the job

+==== Query Parameters
+
+`wait_for_stopped` (optional)::


wait_for_completion for consistency with the other APIs that waits for the completion of the command ?

Also it might be difficult for a user to set up a value for the timeout. So maybe we should make it a boolean and have another parameter (timeout) that could default to 30s ?

Yeah, I was hoping to avoid two parameters, since configuring a timeout without setting wait_for_completion to true won't do anything. But I can see how it's a strange setup. I could go either way

The two params solution is consistent with the GetTask API but I don't have a strong opinion here either.

jimczi · 2018-10-24T20:28:18Z

...in/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportStopRollupAction.java

@@ -52,12 +57,52 @@ protected void taskOperation(StopRollupJobAction.Request request,
                                 ActionListener<StopRollupJobAction.Response> listener) {
        if (jobTask.getConfig().getId().equals(request.getId())) {
            jobTask.stop(listener);


we should not call the listener if waitForStopped is set.

urgh yes, that would have been bad...

jimczi · 2018-10-24T20:29:22Z

...in/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportStopRollupAction.java

@@ -52,12 +57,52 @@ protected void taskOperation(StopRollupJobAction.Request request,
                                 ActionListener<StopRollupJobAction.Response> listener) {
        if (jobTask.getConfig().getId().equals(request.getId())) {
            jobTask.stop(listener);
+            waitForStopped(request, jobTask, listener);


We shouldn't block the current thread. I think we can use the generic thread pool here in order to make sure that we don't block on important threads (network threads). Something like:

threadPool.generic().execute(new AbstractRunnable() { @Override protected void doRun() throws Exception { if (waitForStopped(request, jobTask)) { listener.onResponse(new Stop...); } else { listener.onFailure(new Stop...); } } @Override public void onFailure(Exception e) { listener.onFailure(e); }

Should we just have the task run on the Generic thread from the start? E.g. in the ctor just specify generic instead of same?

I prefer if we explicitly fork to another thread with a comment explaining why we do that. But yes I think it would work if we use generic directly. No strong opinion here either, just a preference ;)

…pped

polyfractal · 2018-11-07T22:29:33Z

@jimczi pending CI passing again I think I addressed your comments

swapped to wait_for_completion, added a timeout. If timeout is omitted the default is 30s like the other task APIs. If wait_for_completion: false then the timeout doesn't do anything
Spawns to a generic threadpool on demand, as discussed.
Refactored so that the listener is wrapped correctly instead of being called twice.

…pped

polyfractal · 2018-11-08T17:45:21Z

Right, trying this again. This time it has the generic threadpool stuff :)

jimczi

LGTM, thanks for iterating !

…pped

polyfractal · 2018-11-13T17:16:26Z

Jenkins, run gradle build tests please

With elastic#34811 the API for stopping rollup jobs got two new url parameters "wait_for_completion" and "timeout". This change adds these to the HLRC APIs as well. Relates to elastic#34811

This adds a `wait_for_completion` flag which allows the user to block the Stop API until the task has actually moved to a stopped state, instead of returning immediately. If the flag is set, a `timeout` parameter can be specified to determine how long (at max) to block the API call. If unspecified, the timeout is 30s. If the timeout is exceeded before the job moves to STOPPED, a timeout exception is thrown. Note: this is just signifying that the API call itself timed out. The job will remain in STOPPING and evenutally flip over to STOPPED in the background. If the user asks the API to block, we move over the the generic threadpool so that we don't hold up a networking thread.

With #34811 the API for stopping rollup jobs got two new url parameters "wait_for_completion" and "timeout". This change adds these to the HLRC APIs as well. Relates to #34811

$polyfractal$

$@polyfractal$ polyfractal added >enhancement v7.0.0 :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v6.5.0 labels Oct 24, 2018

$@polyfractal$ polyfractal requested review from colings86 and jimczi October 24, 2018 15:34

$@polyfractal$

checkstyle

e9c05c3

jimczi requested changes Oct 24, 2018

View reviewed changes

colings86 added v6.6.0 and removed v6.5.0 labels Oct 25, 2018

polyfractal added 5 commits October 29, 2018 14:09

$@polyfractal$

Replace with wait_for_completion and timeout

a288fec

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_wait_for_sto…

4814b39

…pped

$@polyfractal$

checkstyle

eced393

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_wait_for_sto…

fdd2859

…pped

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_wait_for_sto…

55157e5

…pped

polyfractal added 2 commits November 8, 2018 11:32

$@polyfractal$

Switch to generic threadpool when blocking

e7c70c5

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_wait_for_sto…

3fb6b86

…pped

jimczi approved these changes Nov 8, 2018

View reviewed changes

polyfractal added 2 commits November 12, 2018 15:00

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_wait_for_sto…

437837a

…pped

$@polyfractal$

Update yaml skip version, now in 6.6

46a37d1

$@polyfractal$ polyfractal changed the title ~~[Rollup] Add wait_for_stopped option to StopRollupJob API~~ [Rollup] Add wait_for_completion option to StopRollupJob API Nov 13, 2018

$@polyfractal$ polyfractal merged commit c346a0f into elastic:master Nov 13, 2018

cbuescher mentioned this pull request Nov 14, 2018

HLRC: Add parameters to stopRollupJob API #35545

Merged

hendrikmuhs mentioned this pull request Dec 17, 2018

[ML-DataFrame] add a wait_for_completion option to the stop data frame api #36701

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rollup] Add `wait_for_completion` option to StopRollupJob API #34811

[Rollup] Add `wait_for_completion` option to StopRollupJob API #34811

$@polyfractal$ polyfractal commented Oct 24, 2018 •

edited

elasticmachine commented Oct 24, 2018

polyfractal commented Oct 24, 2018

jimczi left a comment

jimczi Oct 24, 2018

jimczi Oct 24, 2018

$@polyfractal$ polyfractal Oct 26, 2018

jimczi Oct 26, 2018

jimczi Oct 24, 2018

$@polyfractal$ polyfractal Oct 26, 2018

jimczi Oct 24, 2018

$@polyfractal$ polyfractal Oct 26, 2018

jimczi Oct 26, 2018

$@polyfractal$ polyfractal Oct 29, 2018

polyfractal commented Nov 7, 2018

polyfractal commented Nov 8, 2018

jimczi left a comment

polyfractal commented Nov 13, 2018

[Rollup] Add wait_for_completion option to StopRollupJob API #34811

[Rollup] Add wait_for_completion option to StopRollupJob API #34811

Conversation

polyfractal commented Oct 24, 2018 • edited

elasticmachine commented Oct 24, 2018

polyfractal commented Oct 24, 2018

jimczi left a 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

polyfractal commented Nov 7, 2018

polyfractal commented Nov 8, 2018

jimczi left a comment

Choose a reason for hiding this comment

polyfractal commented Nov 13, 2018

[Rollup] Add `wait_for_completion` option to StopRollupJob API #34811

[Rollup] Add `wait_for_completion` option to StopRollupJob API #34811

$@polyfractal$ polyfractal commented Oct 24, 2018 •

edited