IGNITE-26646 Use MessageSerializer for PartitionUpdateCountersMessage #12402

chesnokoff · 2025-10-08T19:55:58Z

Thank you for submitting the pull request to the Apache Ignite.

In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:

The Contribution Checklist

There is a single JIRA ticket related to the pull request.
The web-link to the pull request is attached to the JIRA ticket.
The JIRA ticket has the Patch Available state.
The pull request body describes changes that have been made.
The description explains WHAT and WHY was made instead of HOW.
The pull request title is treated as the final commit message.
The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
A reviewer has been mentioned through the JIRA comments
(see the Maintainers list)
The pull request has been checked by the Teamcity Bot and
the green visa attached to the JIRA ticket (see TC.Bot: Check PR)

Notes

If you need any help, please email [email protected] or ask anу advice on http://asf.slack.com #ignite channel.

…niteIoCommunicationMessageSerializationTest#testMessageSerializationAndDeserializationConsistency

…utGetRestart

chesnokoff · 2026-01-19T07:23:59Z

.../apache/ignite/internal/processors/cache/distributed/dht/PartitionUpdateCountersMessage.java

+        if (data == null)
+            return new byte[0];


IgniteIoCommunicationMessageSerializationTest#testMessageSerializationAndDeserializationConsistency fails without null check

Based on the fact that data is initialized in the constructor and cannot be null, I suggest removing this check and adding to IgniteIoCommunicationMessageSerializationTest#initializeMessage:

if (msg instanceof PartitionUpdateCountersMessage) FieldUtils.writeField(msg, "data", new byte[0], true);

chesnokoff · 2026-01-19T07:24:26Z

.../apache/ignite/internal/processors/cache/distributed/dht/PartitionUpdateCountersMessage.java

+     */
+    public void payload(byte[] payload) {
+        data = payload;
+        size = data == null ? 0 : data.length / ITEM_SIZE;


IgniteCachePutGetRestartAbstractTest#testTxPutGetRestart fails without null check

MessageReader#readByteArray returns null when it couldn't fully extract the entire payload from the buffer because the complete byte array didn't fit into the read buffer.

Previously, we were setting the size after successfully reading the whole byte array using the reader.

In my opinion, the current logic seems fine.

wernerdv · 2026-01-19T08:22:51Z

.../apache/ignite/internal/processors/cache/distributed/dht/PartitionUpdateCountersMessage.java

    /** */
-    private byte data[];
+    @Order(0)
+    private int cacheId;


Optional change - usually we set Order according to the declaration order of fields in the class.
I would revert it, but it's up to you.

I changed the field declaration order because writeTo/readFrom wrote the cacheId first and the data afterward.

@shishkovilja wdyt?

@chesnokoff , I agree with @wernerdv . It is not necessary to refactor order of fieds.

…ssageSerializationAndDeserializationConsistency

shishkovilja · 2026-01-19T12:00:29Z

.../apache/ignite/internal/processors/cache/distributed/dht/PartitionUpdateCountersMessage.java

    /** */
-    private byte data[];
+    @Order(0)
+    private int cacheId;


@chesnokoff , I agree with @wernerdv . It is not necessary to refactor order of fieds.

shishkovilja · 2026-01-19T12:02:21Z

.../apache/ignite/internal/processors/cache/distributed/dht/PartitionUpdateCountersMessage.java

+     * @return Payload.
+     */
+    public byte[] payload() {
+        return Arrays.copyOf(data, size * ITEM_SIZE);


Possbile NPE here.
Also, we should ensure that it could not lead to a potential performance drop.

@chesnokoff , see an example:

Suggested change

return Arrays.copyOf(data, size * ITEM_SIZE);

return F.isEmpty(data) ? null : Arrays.copyOf(data, size * ITEM_SIZE);

shishkovilja · 2026-01-19T12:20:12Z

...he/ignite/internal/managers/communication/IgniteIoCommunicationMessageSerializationTest.java

        if (msg instanceof NodeIdMessage)
            FieldUtils.writeField(msg, "nodeId", UUID.randomUUID(), true);

+        if (msg instanceof PartitionUpdateCountersMessage)


Do we need this change? NPE should be properly fixed in a message.

I suggested removing the null check from the message and adding it to the test.
See #12402 (comment)
But the final decision is up to you.

I suggested removing the null check from the message and adding it to the test. See #12402 (comment) But the final decision is up to you.

We can safely send null for empty or null array. BUT, as I see now, message will be in incorrect state if data is null.

So, you are right. Let's keep it.

shishkovilja · 2026-01-19T14:24:51Z

.../apache/ignite/internal/processors/cache/distributed/dht/PartitionUpdateCountersMessage.java

+    /**
+     * @return Payload.
+     */
+    public byte[] payload() {


May be just data?

Suggested change

public byte[] payload() {

public byte[] data() {

shishkovilja · 2026-01-19T14:25:28Z

.../apache/ignite/internal/processors/cache/distributed/dht/PartitionUpdateCountersMessage.java


    /** */
-    private int cacheId;
+    @Order(value = 1, method = "payload")


Suggested change

@Order(value = 1, method = "payload")

@Order(1)

(rename getter and setter to data)

shishkovilja · 2026-01-19T14:28:00Z

.../apache/ignite/internal/processors/cache/distributed/dht/PartitionUpdateCountersMessage.java

+    @Order(0)
+    private int cacheId;

    /** */


Suggested change

/** */

/** Byte representation of partition counters. */

chesnokoff force-pushed the ignite-26646 branch 2 times, most recently from 42034b2 to 3f54423 Compare November 27, 2025 11:11

chesnokoff force-pushed the ignite-26646 branch 3 times, most recently from c0db299 to 2aac42e Compare January 14, 2026 22:14

IGNITE-26646 Use MessageSerializer for PartitionUpdateCountersMessage

8bbcce0

chesnokoff force-pushed the ignite-26646 branch from 2aac42e to 8bbcce0 Compare January 14, 2026 22:16

chesnokoff added 2 commits January 16, 2026 00:51

IGNITE-26646 Fix org.apache.ignite.internal.managers.communication.Ig…

766c1f8

…niteIoCommunicationMessageSerializationTest#testMessageSerializationAndDeserializationConsistency

IGNITE-26646 Add fix for IgniteCachePutGetRestartAbstractTest#testTxP…

e06b8e2

…utGetRestart

chesnokoff commented Jan 19, 2026

View reviewed changes

wernerdv reviewed Jan 19, 2026

View reviewed changes

IGNITE-26646 Fix IgniteIoCommunicationMessageSerializationTest#testMe…

a3890db

…ssageSerializationAndDeserializationConsistency

shishkovilja requested changes Jan 19, 2026

View reviewed changes

	return Arrays.copyOf(data, size * ITEM_SIZE);
	return F.isEmpty(data) ? null : Arrays.copyOf(data, size * ITEM_SIZE);

IGNITE-26646 Use MessageSerializer for PartitionUpdateCountersMessage #12402

Are you sure you want to change the base?

IGNITE-26646 Use MessageSerializer for PartitionUpdateCountersMessage #12402

Conversation

chesnokoff commented Oct 8, 2025

The Contribution Checklist

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chesnokoff Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishkovilja Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chesnokoff Jan 19, 2026 •

edited

Loading

shishkovilja Jan 19, 2026 •

edited

Loading