You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "chesnokoff (via GitHub)" <gi...@apache.org> on 2026/01/14 12:32:49 UTC
[PR] IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions [ignite]
chesnokoff opened a new pull request, #12634:
URL: https://github.com/apache/ignite/pull/12634
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](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions [ignite]
Posted by "chesnokoff (via GitHub)" <gi...@apache.org>.
chesnokoff closed pull request #12634: IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions
URL: https://github.com/apache/ignite/pull/12634
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12634:
URL: https://github.com/apache/ignite/pull/12634#discussion_r2710949970
##########
modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSessionDifferentSerializationTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.tcp;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.Socket;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.X;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.spi.discovery.tcp.messages.TcpDiscoveryHandshakeRequest;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+/** Tests Java serialization header detection in discovery messages. */
+public class TcpDiscoveryIoSessionDifferentSerializationTest extends GridCommonAbstractTest {
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+ }
+
+ /** */
+ @Test
+ public void testDetectJavaObjectStreamHeader() throws Exception {
+ IgniteEx grid = startGrid(0);
Review Comment:
I suggest to write a full test with server nodes of 2 appropriate versions: 2.17 and RU-supporting 2.18.
##########
modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSessionDifferentSerializationTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.tcp;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.Socket;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.X;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.spi.discovery.tcp.messages.TcpDiscoveryHandshakeRequest;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+/** Tests Java serialization header detection in discovery messages. */
+public class TcpDiscoveryIoSessionDifferentSerializationTest extends GridCommonAbstractTest {
Review Comment:
If we introduce a new test like this, let's rename to something like 'TcpDiscoveryDifferentClusterVersionsTest' and fix the javadoc respectively.
##########
modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSessionDifferentSerializationTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.tcp;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.Socket;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.X;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.spi.discovery.tcp.messages.TcpDiscoveryHandshakeRequest;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+/** Tests Java serialization header detection in discovery messages. */
+public class TcpDiscoveryIoSessionDifferentSerializationTest extends GridCommonAbstractTest {
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+ }
+
+ /** */
+ @Test
+ public void testDetectJavaObjectStreamHeader() throws Exception {
+ IgniteEx grid = startGrid(0);
+
+ TcpDiscoverySpi spi = (TcpDiscoverySpi)grid.configuration().getDiscoverySpi();
+
+ TcpDiscoveryHandshakeRequest req = new TcpDiscoveryHandshakeRequest();
Review Comment:
We should also check the the behaviour with `TcpDiscoveryPingRequest` / `TcpDiscoveryPingResponse`. WDYT?
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -168,7 +168,16 @@ <T> T readMessage() throws IgniteCheckedException, IOException {
try {
if (MESSAGE_SERIALIZATION != serMode) {
- detectSslAlert(serMode, in);
+ byte[] hdr = new byte[4];
+ hdr[0] = serMode;
+
+ int read = in.readNBytes(hdr, 1, 3);
+
+ if (read < 3)
+ throw new EOFException();
+
+ detectSslAlert(hdr);
+ detectJavaObjectStreamHeader(hdr);
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12634:
URL: https://github.com/apache/ignite/pull/12634#discussion_r2732180901
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/spi/discovery/TcpDiscoveryDifferentClusterVersionsTest.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.compatibility.spi.discovery;
+
+import org.apache.ignite.compatibility.IgniteReleasedVersion;
+import org.apache.ignite.compatibility.testframework.junits.IgniteCompatibilityAbstractTest;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.apache.ignite.testframework.LogListener;
+import org.junit.Test;
+
+/** */
+public class TcpDiscoveryDifferentClusterVersionsTest extends IgniteCompatibilityAbstractTest {
+ /** */
+ private static final String SER_MODE_MSG = "serMode byte is expected";
Review Comment:
I'm not sure this message is clear to a user. We might say something of an unexpected bytes/reading, old protocol version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12634:
URL: https://github.com/apache/ignite/pull/12634#discussion_r2731896734
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/ClientImpl.java:
##########
@@ -1194,6 +1194,7 @@ private void forceStopRead() throws InterruptedException {
try {
TcpDiscoveryIoSession ses = createSession(sock);
+ ses.switchToFastReader();
Review Comment:
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()`
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -163,63 +176,123 @@ void writeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException
* @throws IgniteCheckedException If deserialization fails.
*/
<T> T readMessage() throws IgniteCheckedException, IOException {
- byte serMode = (byte)in.read();
+ return reader.readMessage();
+ }
- if (JAVA_SERIALIZATION == serMode)
- return U.unmarshal(spi.marshaller(), in, clsLdr);
+ /** Switches reader to fast mode after initial validated read. */
+ public void switchToFastReader() {
+ reader = fastReader;
+ }
- try {
- if (MESSAGE_SERIALIZATION != serMode) {
- detectSslAlert(serMode, in);
+ /** Base discovery message reader. */
+ private abstract class DiscoveryMessageReader {
Review Comment:
Do we need this abstract class? I would use smth. like `DefaultReader` (not abstract) and `CheckedReader`.
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -163,63 +176,123 @@ void writeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException
* @throws IgniteCheckedException If deserialization fails.
*/
<T> T readMessage() throws IgniteCheckedException, IOException {
- byte serMode = (byte)in.read();
+ return reader.readMessage();
+ }
- if (JAVA_SERIALIZATION == serMode)
- return U.unmarshal(spi.marshaller(), in, clsLdr);
+ /** Switches reader to fast mode after initial validated read. */
+ public void switchToFastReader() {
+ reader = fastReader;
+ }
- try {
- if (MESSAGE_SERIALIZATION != serMode) {
Review Comment:
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.
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -163,63 +176,123 @@ void writeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException
* @throws IgniteCheckedException If deserialization fails.
*/
<T> T readMessage() throws IgniteCheckedException, IOException {
- byte serMode = (byte)in.read();
+ return reader.readMessage();
+ }
- if (JAVA_SERIALIZATION == serMode)
- return U.unmarshal(spi.marshaller(), in, clsLdr);
+ /** Switches reader to fast mode after initial validated read. */
+ public void switchToFastReader() {
+ reader = fastReader;
+ }
- try {
- if (MESSAGE_SERIALIZATION != serMode) {
- detectSslAlert(serMode, in);
+ /** Base discovery message reader. */
+ private abstract class DiscoveryMessageReader {
+ /**
+ * Reads the next discovery message from the socket input stream.
+ *
+ * @param <T> Type of the expected message.
+ * @return Deserialized message instance.
+ * @throws IgniteCheckedException If deserialization fails.
+ */
+ abstract <T> T readMessage() throws IgniteCheckedException, IOException;
- // IOException type is important for ServerImpl. It may search the cause (X.hasCause).
- // The connection error processing behavior depends on it.
- throw new IOException("Received unexpected byte while reading discovery message: " + serMode);
- }
-
- Message msg = spi.messageFactory().create(makeMessageType((byte)in.read(), (byte)in.read()));
+ /** Reads serialized discovery message body when {@link #MESSAGE_SERIALIZATION} is used. */
+ protected <T> T readDiscoveryMessageBody() throws IgniteCheckedException {
+ try {
+ Message msg = spi.messageFactory().create(makeMessageType((byte)in.read(), (byte)in.read()));
- msgReader.reset();
- msgReader.setBuffer(msgBuf);
+ msgReader.reset();
+ msgReader.setBuffer(msgBuf);
- MessageSerializer msgSer = spi.messageFactory().serializer(msg.directType());
+ MessageSerializer msgSer = spi.messageFactory().serializer(msg.directType());
- boolean finished;
+ boolean finished;
- do {
- msgBuf.clear();
+ do {
+ msgBuf.clear();
- int read = in.read(msgBuf.array(), msgBuf.position(), msgBuf.remaining());
+ int read = in.read(msgBuf.array(), msgBuf.position(), msgBuf.remaining());
- if (read == -1)
- throw new EOFException("Connection closed before message was fully read.");
+ if (read == -1)
+ throw new EOFException("Connection closed before message was fully read.");
- msgBuf.limit(read);
+ msgBuf.limit(read);
- finished = msgSer.readFrom(msg, msgReader);
+ finished = msgSer.readFrom(msg, msgReader);
- // Server Discovery only sends next message to next Server upon receiving a receipt for the previous one.
- // This behaviour guarantees that we never read a next message from the buffer right after the end of
- // the previous message. But it is not guaranteed with Client Discovery where messages aren't acknowledged.
- // Thus, we have to keep the uprocessed bytes read from the socket. It won't return them again.
- if (msgBuf.hasRemaining()) {
- byte[] unprocessedReadTail = new byte[msgBuf.remaining()];
+ // Server Discovery only sends next message to next Server upon receiving a receipt for the previous one.
+ // This behaviour guarantees that we never read a next message from the buffer right after the end of
+ // the previous message. But it is not guaranteed with Client Discovery where messages aren't acknowledged.
+ // Thus, we have to keep the uprocessed bytes read from the socket. It won't return them again.
+ if (msgBuf.hasRemaining()) {
+ byte[] unprocessedReadTail = new byte[msgBuf.remaining()];
- msgBuf.get(unprocessedReadTail, 0, msgBuf.remaining());
+ msgBuf.get(unprocessedReadTail, 0, msgBuf.remaining());
- in.attachByteArray(unprocessedReadTail);
+ in.attachByteArray(unprocessedReadTail);
+ }
}
+ while (!finished);
+
+ return (T)msg;
}
- while (!finished);
+ catch (Exception e) {
+ // Keep logic similar to `U.marshal(...)`.
+ if (e instanceof IgniteCheckedException)
+ throw (IgniteCheckedException)e;
- return (T)msg;
+ throw new IgniteCheckedException(e);
+ }
}
- catch (Exception e) {
- // Keep logic similar to `U.marshal(...)`.
- if (e instanceof IgniteCheckedException)
- throw (IgniteCheckedException)e;
- throw new IgniteCheckedException(e);
+ /** Reads 4-byte header for diagnostics. */
+ protected byte[] readHeader(byte serMode) throws IOException {
Review Comment:
Is colled only from a derived class.
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -163,63 +176,123 @@ void writeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException
* @throws IgniteCheckedException If deserialization fails.
*/
<T> T readMessage() throws IgniteCheckedException, IOException {
- byte serMode = (byte)in.read();
+ return reader.readMessage();
+ }
- if (JAVA_SERIALIZATION == serMode)
- return U.unmarshal(spi.marshaller(), in, clsLdr);
+ /** Switches reader to fast mode after initial validated read. */
+ public void switchToFastReader() {
+ reader = fastReader;
+ }
- try {
- if (MESSAGE_SERIALIZATION != serMode) {
- detectSslAlert(serMode, in);
+ /** Base discovery message reader. */
+ private abstract class DiscoveryMessageReader {
Review Comment:
To me, looks as overhead. Too much new code, more complication. We alread have many `if`s, `switch`es 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions [ignite]
Posted by "chesnokoff (via GitHub)" <gi...@apache.org>.
chesnokoff commented on code in PR #12634:
URL: https://github.com/apache/ignite/pull/12634#discussion_r2730556935
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -163,63 +176,123 @@ void writeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException
* @throws IgniteCheckedException If deserialization fails.
*/
<T> T readMessage() throws IgniteCheckedException, IOException {
- byte serMode = (byte)in.read();
+ return reader.readMessage();
+ }
- if (JAVA_SERIALIZATION == serMode)
- return U.unmarshal(spi.marshaller(), in, clsLdr);
+ /** Switches reader to fast mode after initial validated read. */
+ public void switchToFastReader() {
+ reader = fastReader;
+ }
- try {
- if (MESSAGE_SERIALIZATION != serMode) {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions [ignite]
Posted by "chesnokoff (via GitHub)" <gi...@apache.org>.
chesnokoff commented on code in PR #12634:
URL: https://github.com/apache/ignite/pull/12634#discussion_r2730556935
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -163,63 +176,123 @@ void writeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException
* @throws IgniteCheckedException If deserialization fails.
*/
<T> T readMessage() throws IgniteCheckedException, IOException {
- byte serMode = (byte)in.read();
+ return reader.readMessage();
+ }
- if (JAVA_SERIALIZATION == serMode)
- return U.unmarshal(spi.marshaller(), in, clsLdr);
+ /** Switches reader to fast mode after initial validated read. */
+ public void switchToFastReader() {
+ reader = fastReader;
+ }
- try {
- if (MESSAGE_SERIALIZATION != serMode) {
Review Comment:
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27554 Improve discovery handshake diagnostics for incompatible versions [ignite]
Posted by "chesnokoff (via GitHub)" <gi...@apache.org>.
chesnokoff commented on code in PR #12634:
URL: https://github.com/apache/ignite/pull/12634#discussion_r2730556935
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -163,63 +176,123 @@ void writeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException
* @throws IgniteCheckedException If deserialization fails.
*/
<T> T readMessage() throws IgniteCheckedException, IOException {
- byte serMode = (byte)in.read();
+ return reader.readMessage();
+ }
- if (JAVA_SERIALIZATION == serMode)
- return U.unmarshal(spi.marshaller(), in, clsLdr);
+ /** Switches reader to fast mode after initial validated read. */
+ public void switchToFastReader() {
+ reader = fastReader;
+ }
- try {
- if (MESSAGE_SERIALIZATION != serMode) {
Review Comment:
Initially `detectJavaObjectStreamHeader` was placed after `detectSslAlert`. The suggestion was to not make check for `serMode` after connection establishments.
To do that we need to notify session that we do not need that checks. So now higher levels of code call `switchToFastReader` for notification and we change reader. As a result, we optimized one `if` check
But on the other hand for me it looks like really small optimization because work with IO takes much more resources than one simple if. Moreover, it seems to break DIP because now higher levels of code have to know details about implementation of `TcpDiscoveryIoSession`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org