-
Notifications
You must be signed in to change notification settings - Fork 91
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
IGNITE-20645 Make ComputeJob.execute asynchronous #3920
Conversation
public interface ComputeJob<R> { | ||
/** | ||
* Executes the job on an Ignite node. | ||
* | ||
* @param context The execution context. | ||
* @param args Job arguments. | ||
* @return Job result. | ||
* @return Job future. Can be null if the job is synchronous and does not return any result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance reasons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Very small performance improvement
- A bit easier for the user at the expense of a one-line check in Ignite code
Do you think we should disallow nulls?
public interface ComputeJob<R> { | ||
/** | ||
* Executes the job on an Ignite node. | ||
* | ||
* @param context The execution context. | ||
* @param args Job arguments. | ||
* @return Job result. | ||
* @return Job future. Can be null if the job is synchronous and does not return any result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to have Nullable futute? Mby in this case user should have CF ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question from @isapego above. I don't have a strong opinion, we can require a non-null future.
user should have CF
CF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CF=CompletableFuture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user can return completedFuture()
or null
, it does not make any difference. Let's keep it this way.
...pute/src/main/java/org/apache/ignite/internal/compute/state/InMemoryComputeStateMachine.java
Outdated
Show resolved
Hide resolved
...mpute/src/test/java/org/apache/ignite/internal/compute/loader/JobClassLoaderFactoryTest.java
Outdated
Show resolved
Hide resolved
...mpute/src/test/java/org/apache/ignite/internal/compute/loader/JobClassLoaderFactoryTest.java
Outdated
Show resolved
Hide resolved
...mpute/src/test/java/org/apache/ignite/internal/compute/loader/JobClassLoaderFactoryTest.java
Outdated
Show resolved
Hide resolved
# Conflicts: # modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientComputeTest.java
@@ -93,7 +93,8 @@ public TaskExecutionInternal( | |||
splitExecution = executorService.submit( | |||
() -> { | |||
MapReduceTask<R> task = instantiateTask(taskClass); | |||
return new SplitResult<>(task, task.split(context, args)); | |||
|
|||
return completedFuture(new SplitResult<>(task, task.split(context, args))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also make MapReduceTask.split
and reduce
asynchronous?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.../apache/ignite/client/handler/requests/table/ClientStreamerWithReceiverBatchSendRequest.java
Outdated
Show resolved
Hide resolved
.../apache/ignite/client/handler/requests/table/ClientStreamerWithReceiverBatchSendRequest.java
Outdated
Show resolved
Hide resolved
modules/client/src/test/java/org/apache/ignite/client/fakes/FakeCompute.java
Outdated
Show resolved
Hide resolved
...tegrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientComputeTest.java
Outdated
Show resolved
Hide resolved
# Conflicts: # modules/compute/src/test/java/org/apache/ignite/internal/compute/executor/ComputeExecutorTest.java
R ComputeJob.execute(...)
toCompletableFuture<R> ComputeJob.executeAsync(...)