You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "valepakh (via GitHub)" <gi...@apache.org> on 2024/12/02 20:02:41 UTC
[PR] IGNITE-23824 Fix compute job native result type deserialization [ignite-3]
valepakh opened a new pull request, #4817:
URL: https://github.com/apache/ignite-3/pull/4817
https://issues.apache.org/jira/browse/IGNITE-23824
The root cause is that we were erroneously using `ByteBuffer.array()` method from the binary tuple buffer to pass back to the client handler node from the target node during the optimization when non-native type was a job argument and native type was a result when running the job remotely.
--
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
Re: [PR] IGNITE-23824 Fix compute job native result type deserialization [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #4817:
URL: https://github.com/apache/ignite-3/pull/4817#discussion_r1867978225
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/ComputeUtils.java:
##########
@@ -509,7 +510,7 @@ static ComputeJobDataHolder marshalAndWrapResult(Object result, @Nullable Marsha
// Value is represented by 3 tuple elements: type, scale, value.
var builder = new BinaryTupleBuilder(3, 3, false);
ClientBinaryTupleUtils.appendObject(builder, result);
- return new ComputeJobDataHolder(NATIVE, builder.build().array());
+ return new ComputeJobDataHolder(NATIVE, IgniteUtils.byteBufferToByteArray(builder.build()));
Review Comment:
The data holder needed only for sending the binary data between nodes. So this particular method `marshalAndWrapResult` is called on the target node before returning the execution result to the client handler node. So this data holder is immediately marshalled using the network messaging, which doesn't support byte buffers. It will probably marshal simple heap byte buffers but definitely not the native buffers.
--
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
Re: [PR] IGNITE-23824 Fix compute job native result type deserialization [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #4817:
URL: https://github.com/apache/ignite-3/pull/4817#discussion_r1868002584
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/ComputeUtils.java:
##########
@@ -509,7 +510,7 @@ static ComputeJobDataHolder marshalAndWrapResult(Object result, @Nullable Marsha
// Value is represented by 3 tuple elements: type, scale, value.
var builder = new BinaryTupleBuilder(3, 3, false);
ClientBinaryTupleUtils.appendObject(builder, result);
- return new ComputeJobDataHolder(NATIVE, builder.build().array());
+ return new ComputeJobDataHolder(NATIVE, IgniteUtils.byteBufferToByteArray(builder.build()));
Review Comment:
This is always a heap buffer, as I understand.
Also, is there something like `writeReplace` in network messaging?
--
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
Re: [PR] IGNITE-23824 Fix compute job native result type deserialization [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #4817:
URL: https://github.com/apache/ignite-3/pull/4817#discussion_r1869467921
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/ComputeUtils.java:
##########
@@ -509,7 +510,7 @@ static ComputeJobDataHolder marshalAndWrapResult(Object result, @Nullable Marsha
// Value is represented by 3 tuple elements: type, scale, value.
var builder = new BinaryTupleBuilder(3, 3, false);
ClientBinaryTupleUtils.appendObject(builder, result);
- return new ComputeJobDataHolder(NATIVE, builder.build().array());
+ return new ComputeJobDataHolder(NATIVE, IgniteUtils.byteBufferToByteArray(builder.build()));
Review Comment:
I decided to revert the split, it doesn't provide any meaningful value since we're copying the bytes anyway in the channel write method and there's no way around that. I clarified some names to better understand whether the holder holds an argument or result and moved on piece of code to the specific method for packing a result.
--
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
Re: [PR] IGNITE-23824 Fix compute job native result type deserialization [ignite-3]
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander merged PR #4817:
URL: https://github.com/apache/ignite-3/pull/4817
--
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
Re: [PR] IGNITE-23824 Fix compute job native result type deserialization [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #4817:
URL: https://github.com/apache/ignite-3/pull/4817#discussion_r1867771757
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/ComputeUtils.java:
##########
@@ -509,7 +510,7 @@ static ComputeJobDataHolder marshalAndWrapResult(Object result, @Nullable Marsha
// Value is represented by 3 tuple elements: type, scale, value.
var builder = new BinaryTupleBuilder(3, 3, false);
ClientBinaryTupleUtils.appendObject(builder, result);
- return new ComputeJobDataHolder(NATIVE, builder.build().array());
+ return new ComputeJobDataHolder(NATIVE, IgniteUtils.byteBufferToByteArray(builder.build()));
Review Comment:
Unnecessary copy, we can refactor `ComputeJobDataHolder` to hold `ByteBuffer` instead of `byte[]`.
--
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
Re: [PR] IGNITE-23824 Fix compute job native result type deserialization [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #4817:
URL: https://github.com/apache/ignite-3/pull/4817#discussion_r1869022137
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/ComputeUtils.java:
##########
@@ -509,7 +510,7 @@ static ComputeJobDataHolder marshalAndWrapResult(Object result, @Nullable Marsha
// Value is represented by 3 tuple elements: type, scale, value.
var builder = new BinaryTupleBuilder(3, 3, false);
ClientBinaryTupleUtils.appendObject(builder, result);
- return new ComputeJobDataHolder(NATIVE, builder.build().array());
+ return new ComputeJobDataHolder(NATIVE, IgniteUtils.byteBufferToByteArray(builder.build()));
Review Comment:
I split the data holder class both for removing this copy and for better logic distinction between the two. The result data holder has custom serialization which writes the buffer to the stream, which still internally does a copy since the `OutputStream` can't write buffer directly so there's a `WritableByteChannel` which does a chunked writes.
--
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