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.

Sign up for GitHub

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

Jump to bottom

IGNITE-17874 Added List and ByteBuffer support to message serialization #1189

Merged
merged 9 commits into from
Oct 12, 2022
Merged

IGNITE-17874 Added List and ByteBuffer support to message serialization #1189

merged 9 commits into from
Oct 12, 2022

Conversation

ibessonov
Copy link
Contributor

@ibessonov
IGNITE-17874 Added List and ByteBuffer support to message serialization
444e22d 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-17874 writeByteBuffer fixed
0b60718 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-17874 Bad fix for "getShort"
810fdf8 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
SammyVimes
SammyVimes requested changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@SammyVimes SammyVimes left a comment

Choose a reason for hiding this comment

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

Needs tests, byte buffer writing seems to be broken

...ache/ignite/internal/network/processor/serialization/MessageCollectionItemTypeConverter.java Show resolved Hide resolved
.../network/src/main/java/org/apache/ignite/internal/network/direct/DirectMarshallingUtils.java Outdated Show resolved Hide resolved
...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Show resolved Hide resolved
...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated Show resolved Hide resolved
...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated Show resolved Hide resolved
...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated Show resolved Hide resolved
...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated Show resolved Hide resolved
...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated Show resolved Hide resolved
...les/network/src/main/java/org/apache/ignite/internal/network/direct/DirectMessageReader.java Outdated Show resolved Hide resolved
@ibessonov
IGNITE-17874 Packed short removed. Some bugs are still unfixed though
ddfe277 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-17874 <checkstyle>
7584a03 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-17874 Stupid bug fixed
1e77ffe 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
sashapolo
sashapolo reviewed Oct 12, 2022
View reviewed changes
modules/network/src/test/java/org/apache/ignite/internal/network/messages/AllTypesMessage.java Show resolved Hide resolved
...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java
@@ -1363,8 +1541,7 @@ public <C extends Collection<?>> C readCollection(MessageCollectionItemType item
* @return Whether array was fully written.
*/
boolean writeArray(Object arr, long off, int len, int bytes) {
assert arr != null;
assert arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
assert arr == null || arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change was necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was, I pass "null" here for direct buffers

Copy link
Contributor

@sashapolo sashapolo Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, didn't notice that, thanks for the explanation. Please add @Nullable here, then

...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated
if (val.isDirect()) {
long address = GridUnsafe.bufferAddress(val);

lastFinished = writeArray(null, address, length, length);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand, why do we bother with all this GridUnsafe shenanigans when writing an array or a ByteBuffer into another ByteBuffer. What's wrong with simply calling put(ByteBuffer)?

Copy link
Contributor Author

@ibessonov ibessonov Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This way I reused code from writeArray, otherwise I'd have to copy it, basically. It does more than just copying, it also manages last position, finished flag and so on

...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated
byteBufferPosition = readInt();
}

if (!lastFinished) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this if should be moved to closer to the readInt call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated Show resolved Hide resolved
...main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java Outdated

lastFinished = writeArray(null, address, length, length);
} else {
byte[] array = val.array();
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, didn't we agree that we should transfer only (limit - position) bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be weird, imo. What if receiver wants to set position to 0? These two numbers are just extra values

@ibessonov
IGNITE-17874 Review comments addressed
f94a819 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-17874 Nullable flag for writeArray parameter
6f5b87f 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-17878 position/limit removed from payload
e7dfd64 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov ibessonov merged commit 2aeb633 into apache:main Oct 12, 2022
@ibessonov ibessonov deleted the ignite-17874 branch October 12, 2022 11:09
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Mar 18, 2023
@ibessonov @lowka
IGNITE-17874 Added List and ByteBuffer support to message serializati…
2d88fa0 
…on (apache#1189)
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Apr 19, 2023
@ibessonov @lowka
IGNITE-17874 Added List and ByteBuffer support to message serializati…
90d1cf3 
…on (apache#1189)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sashapolo sashapolo sashapolo approved these changes

@SammyVimes SammyVimes SammyVimes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ibessonov @sashapolo @SammyVimes