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

Rename Task.blocking to Task.callableInExecutor #292

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

junchuanwang
Copy link
Contributor

@junchuanwang junchuanwang commented Jan 29, 2021

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.callableInExecutor is a better user-facing name. It means to create a task that would create "a callable that run in executor".

return runInExecutor("runInExecutor: " + _taskDescriptor.getDescription(callable.getClass().getName()), callable, executor);
}

@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add java doc to redirect to new method

@@ -1313,7 +1312,7 @@
*
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update blocking to runInExecutor. same for other places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here the word blocking does not need to be replaced.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update test name

Copy link

@karthikbalasub karthikbalasub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on the replacement to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udpated

}


@Deprecated

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on the replacement to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udpated

@junchuanwang
Copy link
Contributor Author

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" ?

@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"?

@junchuanwang junchuanwang changed the title Rename Task.blocking to Task.runInExecutor Rename Task.blocking to Task.callableInExecutor Jan 29, 2021
@evanw555
Copy link
Contributor

  1. In the method body of the new method, I still see a blockingTask.getShallowTraceBuilder().setTaskType(TaskType.BLOCKING.getName());, is this something that may cause confusion or inconsistency, since it's no longer Task.blocking yet it uses TaskType.BLOCKING?
  2. On Karthik's point, the fact that there's an executor argument may be enough of a hint. If we're saying it's just the same as callable but with an executor, can we not just overload Task.callable? Task.callable(name, callable, executor)? May not make sense to pack redundant info into the method name. This would arguably be cleaner.

@junchuanwang
Copy link
Contributor Author

@karthikbalasub @evanw555 I do see benfit of using Task.callable(name, callable, executor) (it is cleaner).

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 asyncCallableTask, than the Task.blocking. So we are kind of reverting things back

@junchuanwang
Copy link
Contributor Author

Link to #289

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.

4 participants