Skip to content

Support scheduled commands in current context #17077

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

Closed
wants to merge 0 commits into from

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 11, 2016

Adds support for scheduling commands to run at a later time on another
thread pool in the current thread's context:

Runnable someCommand = () -> {System.err.println("Demo");};
someCommand = threadPool.getThreadContext().preserveContext(someCommand);
threadPool.schedule(timeValueMinutes(1), Names.GENERAL, someCommand);

This happens automatically for calls to threadPool.execute but schedule
and scheduleWithFixedDelay don't do that, presumably because scheduled
tasks are usually context-less. Rather than preserve the current context
on all scheduled tasks this just makes it possible to preserve it using
the syntax above.

To make this all go it moves the Runnables that wrap the commands from
EsThreadPoolExecutor into ThreadContext.

This, or something like it, is required to support reindex throttling.

@nik9000 nik9000 added >enhancement :Core/Infra/Core Core issues without another label v5.0.0-alpha1 labels Mar 11, 2016
@nik9000
Copy link
Member Author

nik9000 commented Mar 11, 2016

I'm open to hearing that this is the wrong way to do it. I tried initially just interacting directly with the ThreadContext in reindex and it worked fine but the wrapping that EsThreadPoolExecutor did had such nice error handling that I wanted to steal it for us in reindexing. And copy and pasting code makes me sad, so we, yeah, that is how I got here.

@s1monw
Copy link
Contributor

s1monw commented Mar 16, 2016

I like the way this is done. I think when we do issue commands they should by default inherit the context. Folks can still opt out by stashing the context in their runnable, no? Aside of this the cleanups are awesome

@s1monw s1monw self-assigned this Mar 16, 2016
@nik9000
Copy link
Member Author

nik9000 commented Mar 16, 2016

I think when we do issue commands they should by default inherit the context.

I can flip that, sure. I think it is a bigger change but I can see if I can get it working.

@s1monw
Copy link
Contributor

s1monw commented Mar 16, 2016

I can flip that, sure. I think it is a bigger change but I can see if I can get it working.

maybe we get this in as is and we can fix in a followup?

@nik9000
Copy link
Member Author

nik9000 commented Mar 16, 2016

maybe we get this in as is and we can fix in a followup?

I'm happy to do that as well.

@s1monw
Copy link
Contributor

s1monw commented Mar 16, 2016

LGTM then

@nik9000
Copy link
Member Author

nik9000 commented Mar 16, 2016

Broke out the default behavior change into its own issue #17143.

@nik9000 nik9000 closed this Mar 16, 2016
@nik9000
Copy link
Member Author

nik9000 commented Mar 16, 2016

Merged as 80f638b

@clintongormley clintongormley added :Internal and removed :Core/Infra/Core Core issues without another label labels Mar 17, 2016
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.

3 participants