You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ibessonov (via GitHub)" <gi...@apache.org> on 2025/07/01 08:04:02 UTC
[PR] IGNITE-25797 Fix uuid serialization consistency in direct stream [ignite-3]
ibessonov opened a new pull request, #6161:
URL: https://github.com/apache/ignite-3/pull/6161
https://issues.apache.org/jira/browse/IGNITE-25797
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- 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 description of the JIRA ticket explains WHAT was made, WHY and 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.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177367563
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridUnsafe.java:
##########
@@ -728,6 +728,8 @@ public static void putDouble(long addr, double val) {
* @return Short value from byte array.
*/
public static short getShortLittleEndian(byte[] arr, long off) {
+ assert IS_BIG_ENDIAN;
Review Comment:
Please read the javadoc of this method:
> assuming that value should be stored in little-endian byte order and native byte order is big-endian
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177340522
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1189,7 +1236,11 @@ public long readFixedLong() {
if (lastFinished) {
int pos = buf.position();
- val = GridUnsafe.getLong(heapArr, baseOff + pos);
+ if (IS_BIG_ENDIAN) {
Review Comment:
There are two of them, right? If yes, please leave comments in both places
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177372372
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -73,6 +74,23 @@
* {@link DirectByteBufferStream} implementation.
*/
public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
+ /**
+ * There's a binary incompatibility between Apache Ignite 3.0 and later versions due to the bug in 3.0. If someone wants to upgrade from
+ * 3.0 to any other version on AARCH64 architecture or other architecture that was considered "not unaligned" in Ignite 3.0, they need
+ * to set this system property to {@code true}.
+ *
+ * @see #writeUuidLong(long, long)
+ * @see #readUuidLong(long)
+ * @see #unaligned30()
+ */
+ private static final String IGNITE_30_3X_COMPATIBILITY_PROPERTY = "IGNITE_30_3X_UUIDS_SERIALIZATION_COMPATIBILITY";
Review Comment:
> how will the user understand that this is the problem?
We will have to document it
> Maybe we already write somewhere which PDS on which version the node directory was created and then write a warning.
Not going to work, most likely, for a complicated reason that I may explain later if you ask. It will require a lot of thinking to make this approach work
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177405318
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -73,6 +74,23 @@
* {@link DirectByteBufferStream} implementation.
*/
public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
+ /**
+ * There's a binary incompatibility between Apache Ignite 3.0 and later versions due to the bug in 3.0. If someone wants to upgrade from
+ * 3.0 to any other version on AARCH64 architecture or other architecture that was considered "not unaligned" in Ignite 3.0, they need
+ * to set this system property to {@code true}.
+ *
+ * @see #writeUuidLong(long, long)
+ * @see #readUuidLong(long)
+ * @see #unaligned30()
+ */
+ private static final String IGNITE_30_3X_COMPATIBILITY_PROPERTY = "IGNITE_30_3X_UUIDS_SERIALIZATION_COMPATIBILITY";
Review Comment:
Ok, imagine you have 3 nodes with Ignite 3.0. You stop them, replace jar files, and now you start these 3 nodes AND two more nodes with Ignite 3.1.
2 new 3.1 nodes will have to somehow deduce that they are connected to the cluster that was first used in 3.0, and adapt the protocols accordingly. This seems complicated, making that decision based on PDS alone seems completely impossible
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177372372
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -73,6 +74,23 @@
* {@link DirectByteBufferStream} implementation.
*/
public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
+ /**
+ * There's a binary incompatibility between Apache Ignite 3.0 and later versions due to the bug in 3.0. If someone wants to upgrade from
+ * 3.0 to any other version on AARCH64 architecture or other architecture that was considered "not unaligned" in Ignite 3.0, they need
+ * to set this system property to {@code true}.
+ *
+ * @see #writeUuidLong(long, long)
+ * @see #readUuidLong(long)
+ * @see #unaligned30()
+ */
+ private static final String IGNITE_30_3X_COMPATIBILITY_PROPERTY = "IGNITE_30_3X_UUIDS_SERIALIZATION_COMPATIBILITY";
Review Comment:
> how will the user understand that this is the problem?
We will have to document it
> Maybe we already write somewhere which PDS on which version the node directory was created and then write a warning.
Not going to work, most likely, for a complicated reason that I might explain later if you ask. It will require a lot of thinking to make this approach work
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177544020
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1189,7 +1236,11 @@ public long readFixedLong() {
if (lastFinished) {
int pos = buf.position();
- val = GridUnsafe.getLong(heapArr, baseOff + pos);
+ if (IS_BIG_ENDIAN) {
Review Comment:
OK
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177543543
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -73,6 +74,23 @@
* {@link DirectByteBufferStream} implementation.
*/
public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
+ /**
+ * There's a binary incompatibility between Apache Ignite 3.0 and later versions due to the bug in 3.0. If someone wants to upgrade from
+ * 3.0 to any other version on AARCH64 architecture or other architecture that was considered "not unaligned" in Ignite 3.0, they need
+ * to set this system property to {@code true}.
+ *
+ * @see #writeUuidLong(long, long)
+ * @see #readUuidLong(long)
+ * @see #unaligned30()
+ */
+ private static final String IGNITE_30_3X_COMPATIBILITY_PROPERTY = "IGNITE_30_3X_UUIDS_SERIALIZATION_COMPATIBILITY";
Review Comment:
I agree with an idea about warning. I implied that a general solution doesn't seem possible. Just to make that clear
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177320908
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1189,7 +1236,11 @@ public long readFixedLong() {
if (lastFinished) {
int pos = buf.position();
- val = GridUnsafe.getLong(heapArr, baseOff + pos);
+ if (IS_BIG_ENDIAN) {
Review Comment:
Looks just like in all other places, should I comment on this one specifically?
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177064919
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1189,7 +1236,11 @@ public long readFixedLong() {
if (lastFinished) {
int pos = buf.position();
- val = GridUnsafe.getLong(heapArr, baseOff + pos);
+ if (IS_BIG_ENDIAN) {
Review Comment:
Please leave some comments here, this looks very counterintuitive now
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177386014
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -73,6 +74,23 @@
* {@link DirectByteBufferStream} implementation.
*/
public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
+ /**
+ * There's a binary incompatibility between Apache Ignite 3.0 and later versions due to the bug in 3.0. If someone wants to upgrade from
+ * 3.0 to any other version on AARCH64 architecture or other architecture that was considered "not unaligned" in Ignite 3.0, they need
+ * to set this system property to {@code true}.
+ *
+ * @see #writeUuidLong(long, long)
+ * @see #readUuidLong(long)
+ * @see #unaligned30()
+ */
+ private static final String IGNITE_30_3X_COMPATIBILITY_PROPERTY = "IGNITE_30_3X_UUIDS_SERIALIZATION_COMPATIBILITY";
Review Comment:
I would like to ask you to explain.
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177538489
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridUnsafe.java:
##########
@@ -728,6 +728,8 @@ public static void putDouble(long addr, double val) {
* @return Short value from byte array.
*/
public static short getShortLittleEndian(byte[] arr, long off) {
+ assert IS_BIG_ENDIAN;
Review Comment:
I agree, name is confusing, it's one of the reasons I made that mistake in the first place. I will fix these methods later in a separate PR to avoid further confusion, thank you!
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177301111
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridUnsafe.java:
##########
@@ -728,6 +728,8 @@ public static void putDouble(long addr, double val) {
* @return Short value from byte array.
*/
public static short getShortLittleEndian(byte[] arr, long off) {
+ assert IS_BIG_ENDIAN;
Review Comment:
It looks confusing, the method name indicates little endian, but here there is a check that it is big endian.
The same goes for other methods in this class.
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -73,6 +74,23 @@
* {@link DirectByteBufferStream} implementation.
*/
public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
+ /**
+ * There's a binary incompatibility between Apache Ignite 3.0 and later versions due to the bug in 3.0. If someone wants to upgrade from
+ * 3.0 to any other version on AARCH64 architecture or other architecture that was considered "not unaligned" in Ignite 3.0, they need
+ * to set this system property to {@code true}.
+ *
+ * @see #writeUuidLong(long, long)
+ * @see #readUuidLong(long)
+ * @see #unaligned30()
+ */
+ private static final String IGNITE_30_3X_COMPATIBILITY_PROPERTY = "IGNITE_30_3X_UUIDS_SERIALIZATION_COMPATIBILITY";
Review Comment:
Thank you for leaving a comment, but how will the user understand that this is the problem?
When I was sorting out IGNITE-25796 I had to step by step add various logging directly in the code to understand that this is the problem.
I have no idea how to fix this well, let's think together.
Here are my thoughts on this:
1. We can explicitly write somewhere in the documentation that it is not recommended to run on PDS from 3.0 on 3.x if the user has aarch64.
2. Write explicitly somewhere in the release notes that we have such a problem, and it is not recommended to run on PDS from 3.0 on 3.x if the user has aarch64.
3. Maybe we already write somewhere which PDS on which version the node directory was created and then write a warning.
3.1 We can now write somewhere in PDS on which version we created directories with PDS and then write a warning, if there may be such a problem.
What do you think?
We can solve the problem with other tickets of course.
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177432596
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -73,6 +74,23 @@
* {@link DirectByteBufferStream} implementation.
*/
public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
+ /**
+ * There's a binary incompatibility between Apache Ignite 3.0 and later versions due to the bug in 3.0. If someone wants to upgrade from
+ * 3.0 to any other version on AARCH64 architecture or other architecture that was considered "not unaligned" in Ignite 3.0, they need
+ * to set this system property to {@code true}.
+ *
+ * @see #writeUuidLong(long, long)
+ * @see #readUuidLong(long)
+ * @see #unaligned30()
+ */
+ private static final String IGNITE_30_3X_COMPATIBILITY_PROPERTY = "IGNITE_30_3X_UUIDS_SERIALIZATION_COMPATIBILITY";
Review Comment:
Is there support for a cluster with different versions of nodes?
If not, then all nodes will have to start with the same version and we can do the trick I suggested.
--
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-25797 Fix uuid serialization consistency in direct stream [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #6161:
URL: https://github.com/apache/ignite-3/pull/6161#discussion_r2177400433
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridUnsafe.java:
##########
@@ -728,6 +728,8 @@ public static void putDouble(long addr, double val) {
* @return Short value from byte array.
*/
public static short getShortLittleEndian(byte[] arr, long off) {
+ assert IS_BIG_ENDIAN;
Review Comment:
It's good that there is documentation, but the name is very confusing.
--
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