Rename Task.blocking to Task.callableInExecutor#292
Rename Task.blocking to Task.callableInExecutor#292junchuanwang wants to merge 4 commits intomasterfrom
Conversation
| return runInExecutor("runInExecutor: " + _taskDescriptor.getDescription(callable.getClass().getName()), callable, executor); | ||
| } | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
can you add java doc to redirect to new method
| * This method is not appropriate for long running or blocking callables. | ||
| * If callable is long running or blocking use | ||
| * {@link #blocking(String, Callable, Executor) blocking} method. | ||
| * {@link #runInExecutor(String, Callable, Executor) blocking} method. |
There was a problem hiding this comment.
update blocking to runInExecutor. same for other places
There was a problem hiding this comment.
I think here the word blocking does not need to be replaced.
There was a problem hiding this comment.
text will point to #blocking method but link for new method in parsed doc. It will be confusing since we can't remove blocking method
| @@ -94,7 +94,7 @@ public void testAsyncWithContext() { | |||
| public void testBlocking() { | |||
karthikbalasub
left a comment
There was a problem hiding this comment.
Though the naming was bad before, the need to pass executor as an argument would've given enough hint to the user about the behavior. Also, most of the existing users would now be familiar with Task::blocking behavior. So deprecating it might cause push back from such users.
What I'm trying to say is, this is one of those cases where you cannot satisfy every user. It comes down to personal preferences..
Another ambiguity is that "run" might imply the returned task is running already. However, the task still needs to be run by calling Engine::run. Maybe use "runCallableInExecutor" ?
| return runInExecutor("runInExecutor: " + _taskDescriptor.getDescription(callable.getClass().getName()), callable, executor); | ||
| } | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
Comment on the replacement to use.
| } | ||
|
|
||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
Comment on the replacement to use.
@karthikbalasub I am not worried about existing user familiar with Task::blocking thing, becuase I don't believe there will be anyone who would not understand what happened after seen the java doc. Push back? maybe, but I think the team has the final say and that's why I am checking if the team would agree with me. For the naming, maybe just "calalbleInExecutor"? |
|
|
@karthikbalasub @evanw555 I do see benfit of using But the problem is it seems that in ParSeq history, callable and callableInExecutor (or blocking, as its previous name) were made distinct. The fact is in history, there was |
|
Link to #289 |
ParSeq beginner users are often confused about the name of "Task.blocking". It is even more confusing if comparing with other task creating apis: Task.callable, Task.async, Task.action, Task.value.
(I personally think Task.async confusing as well..)
By analogy, Task.callable means to create a task with a callable. Task.value means to create a task with a value. I think
Task.callableInExecutoris a better user-facing name. It means to create a task that would create "a callable that run in executor".