Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make reindex throttling dynamic #17262

Merged
merged 1 commit into from Mar 30, 2016

Conversation

Projects
None yet
4 participants
@nik9000
Copy link
Contributor

commented Mar 22, 2016

This creates a rest end point that lets the user change the throttle of reindex. It takes care to reschedule the task if the user tries to speed up the request. This is important so that users undo throttle values that make reindex sleep forever. We also listen for cancelation and wake up if we are sleeping so that cancelled reindex requests that will sleep for a long time die quickly.

Finally, this adds a field to the task status that is "for how much longer will this request sleep?" I needed it debugging some stuff and see no reason to remove it.

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

This totally needs docs. Also, I'm not sure if this is a good or bad REST location:

        controller.registerHandler(POST, "/_update_by_query/{taskId}/_rethrottle", this);
        controller.registerHandler(POST, "/_reindex/{taskId}/_rethrottle", this);

@imotov I think maybe this is a good one for you to review? Or @dakrone? I'm not sure. A tiny bit of task management stuff, mostly reindex stuff.

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2016

@dakrone can you have a look at this one?

@nik9000 nik9000 force-pushed the nik9000:reindex_throttle_dynamic_2 branch Mar 24, 2016

@clintongormley

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

Hmmm wondering if this should be more generic eg some way of sending messages to tasks via the task management API?

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2016

Hmmm wondering if this should be more generic eg some way of sending messages to tasks via the task management API?

The task management bit of the implementation is basically just that. But at the API level it isn't generic.

@dakrone

View changes

...s/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java Outdated
@@ -173,52 +173,60 @@ void onScrollResponse(TimeValue delay, SearchResponse searchResponse) {
total = min(total, mainRequest.getSize());
}
task.setTotal(total);
task.countThrottle(delay);
threadPool.schedule(delay, ThreadPool.Names.GENERIC, threadPool.getThreadContext().preserveContext(new AbstractRunnable() {
AbstractRunnable prepareBulkRequest = (AbstractRunnable) threadPool.getThreadContext().preserveContext(new AbstractRunnable() {

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 29, 2016

Member

It was confusing to me that you call this prepareBulkRequest but also have a method named prepareBulkRequest, perhaps one of them can be renamed?

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 30, 2016

Author Contributor

Will rename.

@dakrone

View changes

...es/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBaseReindexRestHandler.java Outdated
if (requestsPerSecond == null) {
return null;
}
if ("unlimited".equals(requestsPerSecond)) {

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 29, 2016

Member

I know we sometimes use -1 to indicate disabling something (like refresh interval), does this need to be parsed here as well?

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 30, 2016

Author Contributor

I can allow -1 to mean unlimited as well.

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 30, 2016

Member

Cool, I think we should allow the -1 to it conceptually makes sense :)

@dakrone

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/BulkByScrollTask.java Outdated
* style variables but there isn't an AtomicFloat so we just use a volatile.
*/
private volatile float requestsPerSecond;
private final AtomicReference<DelayedPrepareBulkRequest> delayedPrepareBulkRequest = new AtomicReference<>();

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 29, 2016

Member

Same here with naming, we have delayedPrepareBulkRequest and delayPrepareBulkRequest which are confusing when vars and functions are named so similarly

@dakrone

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/BulkByScrollTask.java Outdated
@Override
protected void doRun() throws Exception {
// Paranoia to prevent furiously rethrottling from running the command multiple times. Without this we totally can.
if (false == oneTime.getAndSet(true)) {

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 29, 2016

Member

This might read better if it were renamed to hasRun initialized with false and then it could be

if (hasRun.compareAndSet(false, true)) {
   prepareBulkRequest.run();
}

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 30, 2016

Author Contributor

Much nicer.

*/
public class RethrottleRequest extends BaseTasksRequest<RethrottleRequest> {
/**
* The throttle to apply to all matching requests in sub-requests per second. 0 means set no throttle and that is the default.

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 29, 2016

Member

As I mentioned above, I do think we should support both 0 and -1 for no throttle to be consistent with our other "disabling" APIs

"params": {
"requests_per_second": {
"type": "float",
"default": 0,

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 29, 2016

Member

Does this then mean that doing curl -XPOST 'localhost:9200/_reindex/1234/_rethrottle?requests_per_second' will set the task to be unlimited requests per sec?

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 30, 2016

Author Contributor

I'm not sure what that'll do. I wrote the default that way to mean "if you don't send the parameter at all it'll default to unlimited." I've not tried sending the parameter without a value - probably an exception parsing the float.

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 30, 2016

Author Contributor

I can make whatever you think is right work. I don't really have an opinion. I can also tell the user it is an error to set the parameter to empty string.

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 30, 2016

Member

Yeah, it'll probably freak out parsing it as an empty string, perhaps we should check Strings.hasText(...) and throw a nicer exception if not?

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2016

Thanks for the review @dakrone ! I pushed another commit that addresses your points - except for the one about the default value which I left a question on inline.

@dakrone

View changes

core/src/main/java/org/elasticsearch/tasks/CancellableTask.java Outdated
@@ -65,4 +66,10 @@ public boolean isCancelled() {
public String getReasonCancelled() {
return reason.get();
}

/**
* Called when the task in cancelled so that it can take any actions that it has to take.

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 30, 2016

Member

in -> is

Might be good to specify also whether it's called before cancellation, or after

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 30, 2016

Author Contributor

e743c2b

}

/**
* Prepare the bulk request. Called on the generic thread pool after some preflight checks have been done one the SearchResponse and any

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 30, 2016

Member

Just curious, but why is this on the generic thread pool instead of an action specific thread pool?

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 30, 2016

Author Contributor

I was going to type a comment but I realized that I'd be better off just pushing 577fc68.

@@ -61,7 +76,11 @@ protected AbstractBaseReindexRestHandler(Settings settings, Client client,
}

protected void execute(RestRequest request, Request internalRequest, RestChannel channel) throws IOException {
internalRequest.setRequestsPerSecond(request.paramAsFloat("requests_per_second", internalRequest.getRequestsPerSecond()));
Float requestsPerSecond = parseRequestsPerSecond(request);
if (requestsPerSecond != null) {

This comment has been minimized.

Copy link
@dakrone

dakrone Mar 30, 2016

Member

Totally up to you (whether you want to or not), but maybe setRequestsPerSecond should handle the null case so you don't have to worry about checking it post-parse? What do you think? (I'm fine either way)

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 30, 2016

Author Contributor

I guess I like it better this way because null doesn't make sense in the context of the other caller. Does that make sense?

@dakrone

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

I left a few more comments (minor ones), but otherwise LGTM, thanks @nik9000

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2016

I left a few more comments (minor ones), but otherwise LGTM, thanks @nik9000

Thank you! I've pushed some more javadoc in response to your comments.

[reindex] Dynamic throttle!
This allows the user to update the reindex throttle on the fly, with changes
that speed up the throttling being applied immediately and changes that
slow down the throttling being applied during the next batch. This means
that if a user throttles reindex in such a way that it tries to sleep for
16 years and then realizes that they've done something wrong then they
can change the throttle and reindex will wake up again. We don't apply
slow downs immediately so we never get in danger of losing the scan context.

Also, if reindex is canceled while it is sleeping (how it honor throttling)
then it'll immediately wake up and cancel itself.

@nik9000 nik9000 force-pushed the nik9000:reindex_throttle_dynamic_2 branch to 78ab6c5 Mar 30, 2016

@nik9000 nik9000 merged commit 78ab6c5 into elastic:master Mar 30, 2016

1 check passed

CLA Commit author has signed the CLA
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.