IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions #12634

chesnokoff · 2026-01-14T12:32:45Z

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.

Vladsz83 · 2026-01-21T03:57:59Z

...ava/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSessionDifferentSerializationTest.java

+import org.junit.Test;
+
+/** Tests Java serialization header detection in discovery messages. */
+public class TcpDiscoveryIoSessionDifferentSerializationTest extends GridCommonAbstractTest {


If we introduce a new test like this, let's rename to something like 'TcpDiscoveryDifferentClusterVersionsTest' and fix the javadoc respectively.

Vladsz83 · 2026-01-21T04:36:52Z

...ava/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSessionDifferentSerializationTest.java

+
+        TcpDiscoverySpi spi = (TcpDiscoverySpi)grid.configuration().getDiscoverySpi();
+
+        TcpDiscoveryHandshakeRequest req = new TcpDiscoveryHandshakeRequest();


We should also check the the behaviour with TcpDiscoveryPingRequest / TcpDiscoveryPingResponse. WDYT?

Vladsz83 · 2026-01-21T04:46:27Z

...ava/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSessionDifferentSerializationTest.java

+    /** */
+    @Test
+    public void testDetectJavaObjectStreamHeader() throws Exception {
+        IgniteEx grid = startGrid(0);


I suggest to write a full test with server nodes of 2 appropriate versions: 2.17 and RU-supporting 2.18.

Vladsz83 · 2026-01-21T04:49:34Z

modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java

+                    throw new EOFException();
+
+                detectSslAlert(hdr);
+                detectJavaObjectStreamHeader(hdr);


How many messages does it affect? TcpDiscoveryHandshakeRequest and TcpDiscoveryPingRequest ? Do we need to check every message? If the additional bytes follow IGNITE_HEADER, we might validate it there. WDYT?

chesnokoff · 2026-01-27T07:06:21Z

modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java

+    }

-        try {
-            if (MESSAGE_SERIALIZATION != serMode) {


Initially, detectJavaObjectStreamHeader was placed after detectSslAlert. The suggestion was to avoid checking serMode after the connection is established.

To achieve this, we need a way to notify session that these checks are no longer required. The current approach is that higher-level code calls switchToFastReader and it swaps the reader implementation. As a result, we eliminate a single if check.

However, to me this looks like a very minor optimization, because I/O dominates the cost compared to a simple conditional. Moreover, it seems to break the DIP, since higher-level code now has to be aware of implementation details of TcpDiscoveryIoSession

If we want not to know about the session implementation, we might create another implementation of TcpDiscoveryIoSession which can check the protocol. Not sure worth it. Or we might bring some flag checkCompatibility to the session. At the first connection, we use session with checkCompatibility=true. On next permanent connection, we might recreate session with checkCompatibility=false. This flag is used in the message serialization.

Vladsz83 · 2026-01-27T12:38:39Z

modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java


-            throw new IgniteCheckedException(e);
+        /** Reads 4-byte header for diagnostics. */
+        protected byte[] readHeader(byte serMode) throws IOException {


Is colled only from a derived class.

Vladsz83 · 2026-01-27T12:46:55Z

modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java

-            if (MESSAGE_SERIALIZATION != serMode) {
-                detectSslAlert(serMode, in);
+    /** Base discovery message reader. */
+    private abstract class DiscoveryMessageReader {


Do we need this abstract class? I would use smth. like DefaultReader (not abstract) and CheckedReader.

Vladsz83 · 2026-01-27T12:53:06Z

modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java

+    }

-        try {
-            if (MESSAGE_SERIALIZATION != serMode) {


If we want not to know about the session implementation, we might create another implementation of TcpDiscoveryIoSession which can check the protocol. Not sure worth it. Or we might bring some flag checkCompatibility to the session. At the first connection, we use session with checkCompatibility=true. On next permanent connection, we might recreate session with checkCompatibility=false. This flag is used in the message serialization.

Vladsz83 · 2026-01-27T12:58:55Z

modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/ClientImpl.java


                try {
                    TcpDiscoveryIoSession ses = createSession(sock);
+                    ses.switchToFastReader();


My overal idea is to use different or flagged createSession() so that it would choose which session to create checked or fast. Same in other places of switchToFastReader()

Vladsz83 · 2026-01-27T13:31:27Z

modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java

-            if (MESSAGE_SERIALIZATION != serMode) {
-                detectSslAlert(serMode, in);
+    /** Base discovery message reader. */
+    private abstract class DiscoveryMessageReader {


To me, looks as overhead. Too much new code, more complication. We alread have many ifs, switches in the message parsing. I belive, one more wouldn't change the roitine. Instead new subclasses, we might bring to the session some flag like checkProtocol. And just recreate session with/without it.

Vladsz83 · 2026-01-27T14:06:52Z

.../org/apache/ignite/compatibility/spi/discovery/TcpDiscoveryDifferentClusterVersionsTest.java

+/** */
+public class TcpDiscoveryDifferentClusterVersionsTest extends IgniteCompatibilityAbstractTest {
+    /** */
+    private static final String SER_MODE_MSG = "serMode byte is expected";


I'm not sure this message is clear to a user. We might say something of an unexpected bytes/reading, old protocol version.

chesnokoff force-pushed the ignite-27554 branch 2 times, most recently from 563a5e3 to e7715bc Compare January 16, 2026 14:16

Vladsz83 reviewed Jan 21, 2026

View reviewed changes

chesnokoff force-pushed the ignite-27554 branch from e7715bc to cb80839 Compare January 22, 2026 06:21

chesnokoff added 6 commits January 27, 2026 09:29

IGNITE-27554 Add check for serMode

55f19bf

IGNITE-27554 Revert junit test

7faea05

IGNITE-27554 Add compatibility test

c3e373b

IGNITE-27554 Add DiscoveryMessageReader

7e35629

IGNITE-27554 Fix style

542b44b

IGNITE-27554 Refactor

6e3812a

chesnokoff force-pushed the ignite-27554 branch from f7a25eb to b782572 Compare January 27, 2026 06:46

chesnokoff commented Jan 27, 2026

View reviewed changes

chesnokoff force-pushed the ignite-27554 branch from c827749 to 4c66586 Compare January 27, 2026 12:09

Vladsz83 reviewed Jan 27, 2026

View reviewed changes

IGNITE-27554 Fix

826b486

chesnokoff force-pushed the ignite-27554 branch from 4c66586 to 826b486 Compare January 27, 2026 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions #12634

IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions #12634

Uh oh!

chesnokoff commented Jan 14, 2026

Uh oh!

Vladsz83 Jan 21, 2026

Uh oh!

Vladsz83 Jan 21, 2026

Uh oh!

Vladsz83 Jan 21, 2026

Uh oh!

Vladsz83 Jan 21, 2026

Uh oh!

chesnokoff Jan 27, 2026 •

edited

Loading

Uh oh!

Vladsz83 Jan 27, 2026

Uh oh!

Vladsz83 Jan 27, 2026

Uh oh!

Vladsz83 Jan 27, 2026

Uh oh!

Vladsz83 Jan 27, 2026

Uh oh!

Vladsz83 Jan 27, 2026

Uh oh!

Vladsz83 Jan 27, 2026

Uh oh!

Vladsz83 Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		TcpDiscoverySpi spi = (TcpDiscoverySpi)grid.configuration().getDiscoverySpi();

		TcpDiscoveryHandshakeRequest req = new TcpDiscoveryHandshakeRequest();

IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions #12634

Are you sure you want to change the base?

IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions #12634

Uh oh!

Conversation

chesnokoff commented Jan 14, 2026

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!

chesnokoff Jan 27, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chesnokoff Jan 27, 2026 •

edited

Loading