You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ptupitsyn (via GitHub)" <gi...@apache.org> on 2023/06/23 14:33:28 UTC
[GitHub] [ignite-3] ptupitsyn opened a new pull request, #2248: IGNITE-19626 .NET: Propagate compute deployment units
ptupitsyn opened a new pull request, #2248:
URL: https://github.com/apache/ignite-3/pull/2248
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ptupitsyn commented on a diff in pull request #2248: IGNITE-19626 .NET: Propagate compute deployment units
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #2248:
URL: https://github.com/apache/ignite-3/pull/2248#discussion_r1241900982
##########
modules/platforms/dotnet/Apache.Ignite/Compute/ICompute.cs:
##########
@@ -15,60 +15,81 @@
* limitations under the License.
*/
-namespace Apache.Ignite.Compute
-{
- using System.Collections.Generic;
- using System.Threading.Tasks;
- using Network;
- using Table;
+namespace Apache.Ignite.Compute;
+
+using System.Collections.Generic;
+using System.Threading.Tasks;
+using Network;
+using Table;
+/// <summary>
+/// Ignite Compute API provides distributed job execution functionality.
+/// </summary>
+public interface ICompute
+{
/// <summary>
- /// Ignite Compute API provides distributed job execution functionality.
+ /// Executes a compute job represented by the given class on one of the specified nodes.
/// </summary>
- public interface ICompute
- {
- /// <summary>
- /// Executes a compute job represented by the given class on one of the specified nodes.
- /// </summary>
- /// <param name="nodes">Nodes to use for the job execution.</param>
- /// <param name="jobClassName">Java class name of the job to execute.</param>
- /// <param name="args">Job arguments.</param>
- /// <typeparam name="T">Job result type.</typeparam>
- /// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
- Task<T> ExecuteAsync<T>(IEnumerable<IClusterNode> nodes, string jobClassName, params object?[]? args);
+ /// <param name="nodes">Nodes to use for the job execution.</param>
+ /// <param name="units">Deployment units.</param>
Review Comment:
Added "can be empty" note. Ideally we should explain what "empty" means here, but I don't have deep enough understanding of https://cwiki.apache.org/confluence/display/IGNITE/IEP-103%3A+Code+Deployment to do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #2248: IGNITE-19626 .NET: Propagate compute deployment units
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #2248:
URL: https://github.com/apache/ignite-3/pull/2248#discussion_r1241761767
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/ComputeComponentImpl.java:
##########
@@ -238,12 +238,15 @@ private void processExecuteRequest(ExecuteRequest executeRequest, String senderC
List<DeploymentUnit> units = toDeploymentUnit(executeRequest.deploymentUnits());
mapClassLoaderExceptions(jobClassLoader(units), executeRequest.jobClassName())
- .thenCompose(context -> {
- return doExecuteLocally(jobClass(context.classLoader(), executeRequest.jobClassName()), executeRequest.args())
- .whenComplete((r, e) -> context.close())
- .handle((result, ex) -> sendExecuteResponse(result, ex, senderConsistentId, correlationId));
- }
- );
+ .whenComplete((context, err) -> {
+ if (err != null) {
+ sendExecuteResponse(null, err, senderConsistentId, correlationId);
+ }
+
+ doExecuteLocally(jobClass(context.classLoader(), executeRequest.jobClassName()), executeRequest.args())
+ .whenComplete((r, e) -> context.close())
Review Comment:
After this change, it seems that there is a possibility that the `context` will not be closed (if first `sendExecuteResponse` on line 243 throws an exception). How about moving the context closure after `whenComplete()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] isapego commented on a diff in pull request #2248: IGNITE-19626 .NET: Propagate compute deployment units
Posted by "isapego (via GitHub)" <gi...@apache.org>.
isapego commented on code in PR #2248:
URL: https://github.com/apache/ignite-3/pull/2248#discussion_r1241734233
##########
modules/platforms/dotnet/Apache.Ignite/Compute/ICompute.cs:
##########
@@ -15,60 +15,81 @@
* limitations under the License.
*/
-namespace Apache.Ignite.Compute
-{
- using System.Collections.Generic;
- using System.Threading.Tasks;
- using Network;
- using Table;
+namespace Apache.Ignite.Compute;
+
+using System.Collections.Generic;
+using System.Threading.Tasks;
+using Network;
+using Table;
+/// <summary>
+/// Ignite Compute API provides distributed job execution functionality.
+/// </summary>
+public interface ICompute
+{
/// <summary>
- /// Ignite Compute API provides distributed job execution functionality.
+ /// Executes a compute job represented by the given class on one of the specified nodes.
/// </summary>
- public interface ICompute
- {
- /// <summary>
- /// Executes a compute job represented by the given class on one of the specified nodes.
- /// </summary>
- /// <param name="nodes">Nodes to use for the job execution.</param>
- /// <param name="jobClassName">Java class name of the job to execute.</param>
- /// <param name="args">Job arguments.</param>
- /// <typeparam name="T">Job result type.</typeparam>
- /// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
- Task<T> ExecuteAsync<T>(IEnumerable<IClusterNode> nodes, string jobClassName, params object?[]? args);
+ /// <param name="nodes">Nodes to use for the job execution.</param>
+ /// <param name="units">Deployment units.</param>
Review Comment:
Can we provide an empty list? Let's add these details to comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ptupitsyn commented on a diff in pull request #2248: IGNITE-19626 .NET: Propagate compute deployment units
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #2248:
URL: https://github.com/apache/ignite-3/pull/2248#discussion_r1241900982
##########
modules/platforms/dotnet/Apache.Ignite/Compute/ICompute.cs:
##########
@@ -15,60 +15,81 @@
* limitations under the License.
*/
-namespace Apache.Ignite.Compute
-{
- using System.Collections.Generic;
- using System.Threading.Tasks;
- using Network;
- using Table;
+namespace Apache.Ignite.Compute;
+
+using System.Collections.Generic;
+using System.Threading.Tasks;
+using Network;
+using Table;
+/// <summary>
+/// Ignite Compute API provides distributed job execution functionality.
+/// </summary>
+public interface ICompute
+{
/// <summary>
- /// Ignite Compute API provides distributed job execution functionality.
+ /// Executes a compute job represented by the given class on one of the specified nodes.
/// </summary>
- public interface ICompute
- {
- /// <summary>
- /// Executes a compute job represented by the given class on one of the specified nodes.
- /// </summary>
- /// <param name="nodes">Nodes to use for the job execution.</param>
- /// <param name="jobClassName">Java class name of the job to execute.</param>
- /// <param name="args">Job arguments.</param>
- /// <typeparam name="T">Job result type.</typeparam>
- /// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
- Task<T> ExecuteAsync<T>(IEnumerable<IClusterNode> nodes, string jobClassName, params object?[]? args);
+ /// <param name="nodes">Nodes to use for the job execution.</param>
+ /// <param name="units">Deployment units.</param>
Review Comment:
Added "can be empty" note. Ideally we should explain what that means, but I don't have deep enough understanding of https://cwiki.apache.org/confluence/display/IGNITE/IEP-103%3A+Code+Deployment to do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ptupitsyn merged pull request #2248: IGNITE-19626 .NET: Propagate compute deployment units
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn merged PR #2248:
URL: https://github.com/apache/ignite-3/pull/2248
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ptupitsyn commented on a diff in pull request #2248: IGNITE-19626 .NET: Propagate compute deployment units
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #2248:
URL: https://github.com/apache/ignite-3/pull/2248#discussion_r1241902746
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/ComputeComponentImpl.java:
##########
@@ -238,12 +238,15 @@ private void processExecuteRequest(ExecuteRequest executeRequest, String senderC
List<DeploymentUnit> units = toDeploymentUnit(executeRequest.deploymentUnits());
mapClassLoaderExceptions(jobClassLoader(units), executeRequest.jobClassName())
- .thenCompose(context -> {
- return doExecuteLocally(jobClass(context.classLoader(), executeRequest.jobClassName()), executeRequest.args())
- .whenComplete((r, e) -> context.close())
- .handle((result, ex) -> sendExecuteResponse(result, ex, senderConsistentId, correlationId));
- }
- );
+ .whenComplete((context, err) -> {
+ if (err != null) {
+ sendExecuteResponse(null, err, senderConsistentId, correlationId);
+ }
+
+ doExecuteLocally(jobClass(context.classLoader(), executeRequest.jobClassName()), executeRequest.args())
+ .whenComplete((r, e) -> context.close())
Review Comment:
We can't close the `context` in `whenComplete` continuation, because it completes earlier than `doExecuteLocally`. So I've added `context.close()` to the error handling block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org