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

Deprecates use of Task in ConfigKey #883

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

aledsage
Copy link
Contributor

@aledsage aledsage commented Nov 7, 2017

Also avoid StackOverflowError in ExecutionContext.getImmediately(task)
When a non-BasicTask is passed in.


The motivation for deprecating setting a ConfigKey as a Task is that we really shouldn't be doing it! It can get very messed up if one tries to getImmediately or get non-blocking (e.g. by a Transformer that is trying to evaluate its result when triggered). That can leave the task in a cancelled state, so all subsequent calls to task.get() (i.e. all calls to config().get(mykey)) will fail!

Instead, we should be using TaskFactory (which is how the DSL evaluation works).

It's good to deprecate this before a major release of 1.0.0.

It would be good to add an overloaded method for setting config to set(ConfigKey<T>, TaskFactory<T>) so that such usage doesn't involve ugly casting to work around the strongly typed generics. We can do that as a stage two, separate from this PR.

Also avoid StackOverflowError in ExecutionContext.getImmediately(task)
When a non-BasicTask is passed in.
@ahgittin
Copy link
Contributor

ahgittin commented Nov 8, 2017

Looks good. Merging.

Agree re set(ConfigKey<T>, TaskFactory<T>). Would also be nice to warn but similarly we can do that in a separate PR, once we've removed other uses. In particular Java calls to DependentConfiguration eg attributeWhenReady will make Task instances so that is an obvious item to fix.

There is related work in #853 which should be built upon as this continues.

@asfgit asfgit merged commit 2e4fc3e into apache:master Nov 8, 2017
asfgit pushed a commit that referenced this pull request Nov 8, 2017
@aledsage aledsage deleted the deprecate-configkey-tasks branch November 17, 2017 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants