-
Notifications
You must be signed in to change notification settings - Fork 1.9k
IGNITE-27684 Update IGNITE_HEADER for discovery protocol #12671
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.
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
base: master
Are you sure you want to change the base?
Conversation
a3b7907 to
8715e94
Compare
| public static final byte[] IGNITE_HEADER_V1 = intToBytes(0x00004747); | ||
|
|
||
| /** Network packet header. */ | ||
| public static final byte[] IGNITE_HEADER_V2 = intToBytes(0x0049474E); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0IGN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need any header? What it used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need a header as a simple magic prefix to quickly reject non-Ignite or wrong-endpoint connections before we try to parse any data. IGNITE_HEADER_v2 makes the new handshake path explicit and helps keep backward compatibility and clearer diagnostics for version mismatches.
|
|
||
| /** Network packet header. */ | ||
| public static final byte[] IGNITE_HEADER = intToBytes(0x00004747); | ||
| public static final byte[] IGNITE_HEADER_V1 = intToBytes(0x00004747); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
00GG
| log.debug("Remote node uses legacy discovery protocol (V1), local expects V2 " + | ||
| "(check Ignite versions / rolling upgrade compatibility) [rmtAddr=" + rmtAddr + | ||
| ", locAddr=" + sock.getLocalSocketAddress() + "]"); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed to send message to old node with legacy protocol. I tried it but new node fails with
Failed to deserialize object with given class loader: jdk.internal.loader.ClassLoaders$AppClassLoader@311d617d at org/apache/ignite/spi/discovery/tcp/ServerImpl.java:6955
It is Caused by: org.apache.ignite.spi.discovery.tcp.messages.TcpDiscoveryAbstractTraceableMessage; local class incompatible: stream classdesc serialVersionUID = 5764055654892291162, local class serialVersionUID = -2609852714099079963
It looks like deserialization is impossible because we changed structure of our messages after refactoring. So it would be challenging to support all legacy formats. As a quick solution, let's just log message about legacy protocol
| public static final byte[] IGNITE_HEADER = intToBytes(0x00004747); | ||
| public static final byte[] IGNITE_HEADER_V1 = intToBytes(0x00004747); | ||
|
|
||
| /** Network packet header. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's describe in the javadoc what is this new header for.
| if (!U.bytesEqual(U.IGNITE_HEADER, 0, data, 0, U.IGNITE_HEADER.length)) { | ||
| if (!U.bytesEqual(U.IGNITE_HEADER_V2, 0, data, 0, U.IGNITE_HEADER_V2.length)) { | ||
| if (U.bytesEqual(U.IGNITE_HEADER_V1, 0, data, 0, U.IGNITE_HEADER_V1.length)) { | ||
| log.warning("Received message with old header."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: 'old header' -> 'the old header'
| if (!Arrays.equals(buf, U.IGNITE_HEADER_V2)) { | ||
| if (Arrays.equals(buf, U.IGNITE_HEADER_V1)) { | ||
| if (log.isDebugEnabled()) | ||
| log.debug("Remote node uses legacy discovery protocol (V1), local expects V2 " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: 'Remote node uses legacy discovery protocol (V1), local expects V2' -> 'Remote node uses legacy discovery protocol (V1) (before the rolling upgrade compatibility). Local node expects V2.'
|
|
||
| if (magicBuf == null) | ||
| magicBuf = new byte[U.IGNITE_HEADER.length]; | ||
| magicBuf = new byte[U.IGNITE_HEADER_V1.length]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to use V2 header here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestion is to keep IGNITE_HEADER unchanged in the communication SPI and update it only in discovery. As I understand, this is not related to the discovery SPI
| } | ||
|
|
||
| /** | ||
| * Compatibility test that ensures previous-version client fails to connect to current server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: 'Tests that connection from client of old version is properly refused.'
|
|
||
| GridTestUtils.assertThrows( | ||
| log, | ||
| () -> startGrid("old-client", OLD_VERSION.toString(), cfg -> cfg.setClientMode(true)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we check only client node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Initial problem was related to connection between client and server node
- Probably it would be a good idea to check server node too, but there is race condition between two nodes to become an initiator. As a result, new node may try to connect to old one and we won't get anything from logs
fe32a7a to
a8474c8
Compare
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
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.