Ignite 27684#2
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Ignite TCP discovery “magic header” and improves diagnostics when an incoming connection doesn’t match the expected protocol header, plus adds a compatibility test to validate behavior when an older-version node attempts to join.
Changes:
- Changed the global
U.IGNITE_HEADERvalue used as the initial network packet header. - Enhanced TCP discovery “unknown connection” logs to mention incompatible nodes and include the received header bytes.
- Added a new compatibility test and wired it into the compatibility basic suite.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/ServerImpl.java | Improves unknown-connection diagnostics in TCP discovery by expanding the message and logging the received header bytes. |
| modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java | Changes the IGNITE_HEADER magic value used across multiple network entry points. |
| modules/compatibility/src/test/java/org/apache/ignite/compatibility/testsuites/IgniteCompatibilityBasicTestSuite.java | Registers the new compatibility test in the basic test suite. |
| modules/compatibility/src/test/java/org/apache/ignite/compatibility/spi/discovery/TcpDiscoveryDifferentClusterVersionsTest.java | Adds a compatibility test asserting an older-version node is rejected and that the expected log message is emitted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.debug("Unknown connection detected (is some other software connecting to " + | ||
| "this Ignite port?" + | ||
| "this Ignite port, or is this an incompatible Ignite node?" + | ||
| (!spi.isSslEnabled() ? " missed SSL configuration?" : "" ) + |
There was a problem hiding this comment.
In the debug log message, the optional SSL hint says "missed SSL configuration?" which looks like a typo and is inconsistent with the warn-level message below ("missing SSL configuration on remote node?"). Consider using consistent, grammatically correct wording in both messages (e.g., "missing SSL configuration"), since this text is now part of the updated diagnostics for unknown/incompatible connections.
| (!spi.isSslEnabled() ? " missed SSL configuration?" : "" ) + | |
| (!spi.isSslEnabled() ? " missing SSL configuration on remote node?" : "" ) + |
| public static Collection<?> client() { | ||
| return List.of(true, false); |
There was a problem hiding this comment.
JUnit4 Parameterized expects each parameter set to be provided as an Object[] (or Object[][]). Returning a Collection here is likely to cause a ClassCastException when the runner tries to interpret each element as an Object[]. Please change this to return a Collection<Object[]> (e.g., [[true], [false]]) or use GridTestUtils.cartesianProduct(List.of(true, false)) to match other compatibility tests in this module.
| public static Collection<?> client() { | |
| return List.of(true, false); | |
| public static Collection<Object[]> client() { | |
| return GridTestUtils.cartesianProduct(List.of(true, false)); |
| /** Setup node closure. */ | ||
| private class ConfigurationClosure implements IgniteInClosure<IgniteConfiguration> { | ||
| /** {@inheritDoc} */ | ||
| @Override public void apply(IgniteConfiguration cfg) { | ||
| cfg.setClientMode(client); | ||
|
|
||
| cfg.setLocalHost("127.0.0.1"); | ||
| TcpDiscoverySpi disco = new TcpDiscoverySpi(); | ||
| disco.setIpFinder(LOCAL_IP_FINDER); | ||
|
|
||
| cfg.setDiscoverySpi(disco); | ||
| } |
There was a problem hiding this comment.
ConfigurationClosure is a non-static inner class, so it implicitly captures the outer test instance. Because compatibility tests serialize the closure to XML via XStream (IgniteCompatibilityNodeRunner.storeToFile) before starting the old-version node in a separate JVM, this will also serialize the outer test fields (including ListeningTestLogger state), which is unnecessary and can make closure serialization/deserialization fragile. Make the closure static (or a top-level class) and pass only the required inputs (e.g., client mode boolean) explicitly.
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.