You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "anton-vinogradov (via GitHub)" <gi...@apache.org> on 2026/07/28 19:56:29 UTC

[PR] IGNITE-28356 Remove hand-written wire conversion from TxLocksRequest and GridNearGetRequest [ignite]

anton-vinogradov opened a new pull request, #13408:
URL: https://github.com/apache/ignite/pull/13408

   ### What
   
   `TxLocksRequest` and `GridNearGetRequest` converted between their logical field and their wire form by hand:
   
   - `TxLocksRequest#txKeys` was mirrored into a companion `IgniteTxKey[] txKeysArr`;
   - `GridNearGetRequest` split its key map into `keys` + `readersFlags` in the constructor and zipped it back lazily in `keyMap()`.
   
   Both conversions are gone. `TxLocksRequest#txKeys` is now an ordinary `@Order` field, and `GridNearGetRequest` declares the map itself, leaving the zipping to the generated marshalling.
   
   ### Why
   
   The conversions existed because a `KeyCacheObject` read off the wire has no hash code yet โ€” it throws until the message is unmarshalled โ€” so a `Set` or a `Map` keyed by one cannot be filled while the message is being read. That cannot be fixed in `hashCode()` itself: it has to match the hash of the resolved user key, which is only recoverable from the bytes for binary keys.
   
   The generated marshalling already resolves the keys before building the map, which is exactly the place where the conversion belongs. And a `Collection` field needs no mechanism at all โ€” the reader fills a list and never hashes anything, which is why `txKeys` needs no companion.
   
   ### Notes for review
   
   - `txKeys()` returns a `Collection` instead of a `Set`. The only consumer, `IgniteTxManager#txLocksInfo`, already accepted a `Collection` and only iterates it.
   - `GridNearGetRequest#keyMap` is rebuilt as a `HashMap` rather than a `LinkedHashMap`, so its iteration order on the receiving side is no longer the wire order. Nothing depends on that order.
   - The reader flags are now always sent; previously they were left out when `addReader` was `false`. That is one byte per key.
   - `partition()` reads whichever of the two fields is populated: it is called before `unmarshal` when dispatching to a stripe, and on an unmarshalled message when a node sends to itself.
   
   Both messages change their wire layout, so the protected-classes check will flag them for rolling upgrade.
   
   ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5120550595

   <!-- ignite-rolling-upgrade-check -->
   ## Possible compatibility issues. Please, check rolling upgrade cases
   
   This PR modifies protected classes (with **Order** annotation).
   Changes to these classes can break rolling upgrade compatibility.
   
   **Affected files:**
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/GenericValueMessage.java`
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryStartRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedTxPrepareRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridNearAtomicFullUpdateRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/KeyedVersionedValue.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/continuous/CacheContinuousQueryEntry.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/service/LazyServiceConfigurationMessage.java`
   


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3681048184


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java:
##########
@@ -67,15 +64,16 @@ public class GridNearGetRequest extends GridCacheIdMessage implements GridCacheD
 
     /** */
     @GridToStringInclude
-    private LinkedHashMap<KeyCacheObject, Boolean> keyMap;
+    @Marshalled(keys = "keys", values = "readersFlags")
+    Map<KeyCacheObject, Boolean> keyMap;

Review Comment:
   After the changes, HashMap will be created instead of LinkedHashMap in `GridNearGetRequestMarshaller#unmarshal`:
   `msg.keyMap = U.newHashMap(msg.keys.size());`
   
   Is there a problem with that?



-- 
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-28356 Get rid of redundant wire companion fields in cache messages [ignite]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5110722845

   <!-- ignite-rolling-upgrade-check -->
   ## Possible compatibility issues. Please, check rolling upgrade cases
   
   This PR modifies protected classes (with **Order** annotation).
   Changes to these classes can break rolling upgrade compatibility.
   
   **Affected files:**
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/GenericValueMessage.java`
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryStartRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedTxPrepareRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/KeyedVersionedValue.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/service/LazyServiceConfigurationMessage.java`
   


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3683127586


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java:
##########
@@ -67,15 +64,16 @@ public class GridNearGetRequest extends GridCacheIdMessage implements GridCacheD
 
     /** */
     @GridToStringInclude
-    private LinkedHashMap<KeyCacheObject, Boolean> keyMap;
+    @Marshalled(keys = "keys", values = "readersFlags")
+    Map<KeyCacheObject, Boolean> keyMap;

Review Comment:
   No problem in practice, though the change is real โ€” thanks for spotting it.
   
   `keyMap()` has exactly one consumer: `GridDhtCacheAdapter#processNearGetRequest:1292` โ†’ `getDhtAsync` โ†’ `GridDhtGetFuture`. None of the steps there needs the order โ€” `init()` passes `keys.keySet()` to the preloader, reader registration is per-entry, and `getDhtAllAsync` takes a key set. And the order dies at the next step anyway, already in master: the local get result goes into `U.newHashMap` (`GridDhtCacheAdapter:730`), `misses` into `new HashMap<>()` (:816), and `GridDhtGetFuture#toEntryInfos` iterates that map to build the response. So the entry order in `GridNearGetResponse` never matched the request order. The near side matches by key, not by position (`CacheDistributedGetFutureAdapter#onResult` โ†’ `createResultMap`, `GridPartitionedGetFuture:600-620`).
   
   `partition()` is unaffected: it runs on the NIO thread before unmarshal, when `keyMap` is still null and `keys` is the wire-order `ArrayList`, so it returns the same partition as master.
   
   I also looked for the reason behind `LinkedHashMap`: `git log -S` traces it to the very first import (`7016f154b20`), then it was carried over mechanically in `832f114e8eb` when `keyBytes` was split into `keys`/`flags`. No ticket, no comment, no test pins it.
   
   One thing worth a follow-up rather than this PR: the `@Order` path *does* derive the implementation from the declared type โ€” `MessageSerializerGenerator:542` emits `linked=true` for a `LinkedHashMap` field and `readMap` then uses `U.newLinkedHashMap`. `MessageMarshallerGenerator` hardcodes `U.newHashMap`/`U.newHashSet` instead, so declaring the field `LinkedHashMap` here does not even compile (I tried: `incompatible types: HashMap cannot be converted to LinkedHashMap`). Teaching the marshaller generator to derive it the same way would remove the trap for future `@Marshalled` fields where the order does matter. Happy to file that separately if you agree.



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3681048184


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java:
##########
@@ -67,15 +64,16 @@ public class GridNearGetRequest extends GridCacheIdMessage implements GridCacheD
 
     /** */
     @GridToStringInclude
-    private LinkedHashMap<KeyCacheObject, Boolean> keyMap;
+    @Marshalled(keys = "keys", values = "readersFlags")
+    Map<KeyCacheObject, Boolean> keyMap;

Review Comment:
   After the changes, HashMap will be created instead of LinkedHashMap in GridNearGetRequestMarshaller#unmarshal:
   `msg.keyMap = U.newHashMap(msg.keys.size());`
   
   Is there a problem with that?



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5118246727

   **[Ignite PR Checker](https://ignite-pr-checker.is-a.dev/?pr=13408)** verdict ยท RunAll build [9236765](https://ci2.ignite.apache.org/build/9236765) ยท 147 suites ran, 0 reused
   
   โŒ **2 blocker(s) in 2 suite(s):**
   - Queries 5: `org.apache.ignite.testsuites.IgniteBinaryCacheQueryTestSuite5: org.apache.ignite.internal.processors.cache.index.DynamicEnableIndexingBasicSelfTest.testEnableDynamicIndexing[hasNear=false,nodeIdx=3,cacheMode=PARTITIONED,atomicityMode=ATOMIC]`
   - Cache 2: `org.apache.ignite.testsuites.IgniteCacheTestSuite2: org.apache.ignite.internal.processors.cache.distributed.near.GridNearTxPrepareDestroyedCacheTest.testPrepareOfDestroyedCacheAnsweredWithErrorResponse`
   
   
   โณ _Auto re-run **#1** in progress โ€” 2 blocker suite(s) re-queued (attempt 1/2). This comment updates when they settle._


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3684328066


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java:
##########
@@ -67,15 +64,16 @@ public class GridNearGetRequest extends GridCacheIdMessage implements GridCacheD
 
     /** */
     @GridToStringInclude
-    private LinkedHashMap<KeyCacheObject, Boolean> keyMap;
+    @Marshalled(keys = "keys", values = "readersFlags")
+    Map<KeyCacheObject, Boolean> keyMap;

Review Comment:
   > Happy to file that separately if you agree.
   Yes, that makes sense!



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java:
##########
@@ -67,15 +64,16 @@ public class GridNearGetRequest extends GridCacheIdMessage implements GridCacheD
 
     /** */
     @GridToStringInclude
-    private LinkedHashMap<KeyCacheObject, Boolean> keyMap;
+    @Marshalled(keys = "keys", values = "readersFlags")
+    Map<KeyCacheObject, Boolean> keyMap;

Review Comment:
   > Happy to file that separately if you agree.
   
   Yes, that makes sense!



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3683129556


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java:
##########
@@ -35,14 +34,9 @@ public class TxLocksRequest extends GridCacheMessage {
     long futId;
 
     /** Tx keys. */
-    @GridToStringInclude
-    @Marshalled("txKeysArr")
-    Set<IgniteTxKey> txKeys;
-
-    /** Array of txKeys from {@link #txKeys}. Used during marshalling and unmarshalling. */
-    @GridToStringExclude
     @Order(1)
-    IgniteTxKey[] txKeysArr;
+    @GridToStringInclude
+    Collection<IgniteTxKey> txKeys;

Review Comment:
   We can't โ€” and it turned out to be the more interesting answer, so let me lay it out.
   
   The codegen does honour the declared type on the `@Order` path: `MessageSerializerGenerator:551` emits `set=true` for a `Set` field, and `DirectByteBufferStream:1659` then builds `U.newHashSet` instead of an `ArrayList`. The catch is *when*: the set is filled inside the read loop (`col.add(item)`, :1667), and at that moment `IgniteTxKey#hashCode()` โ†’ `KeyCacheObjectImpl#hashCode()` throws `CacheObjectNotResolvedException`, because the reader created the key as `new KeyCacheObjectImpl(null, bytes, -1)` (`CacheObjectBinaryProcessorImpl:1299`). The value is resolved later, in the generated marshaller's `unmarshal`.
   
   I verified it instead of reasoning about it: declaring the field `Set<IgniteTxKey>` compiles, but `TxDeadlockDetectionMessageMarshallingTest` then fails with `GridDirectParser: Failed to read message ... CacheObjectNotResolvedException` and the NIO session closed. With `Collection` it passes.
   
   That is exactly what the companion array in master was for โ€” the generated ELEMENTS code does `for (e : arr) { unmarshal(e); col.add(e); }`, i.e. hashing deferred until the cache objects are resolved. The same pattern is still visible in `TxLocksResponse` for `nearTxKeyLocks`: `put(k, v)` happens only after `MessageMarshalling.unmarshal(k, ...)`.
   
   So keeping `Set` in the declaration means keeping the companion array. The wire is byte-identical either way (`writeCollection` and `writeObjectArray` both emit size + elements, and the `set` flag never reaches the wire), so the only thing it buys is the type-level guarantee โ€” and nothing depends on it. The sole consumers are `IgniteTxManager#hasKeys` (plain iteration) and `TxDeadlockDetection#mapTxKeys` (iteration into per-node `HashSet`s and a `UniqueDeque`): no `contains()` over these keys, no collection `equals`, no receive-side dedup needed. The sender-side dedup stays and does real work โ€” `TxLocksRequest`'s constructor still takes a `Set`, and `TxLocksResponse#addKey` builds one, which matters because the same key can be added from two different transactions.
   
   Expanded the field comment to state the reason rather than just assert "a set", so the next reader doesn't have to ask.



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksResponse.java:
##########
@@ -43,21 +43,16 @@ public class TxLocksResponse extends GridCacheMessage {
     @Marshalled(keys = "nearTxKeysArr", values = "locksArr")
     final Map<IgniteTxKey, List<TxLock>> nearTxKeyLocks = new HashMap<>();
 
-    /** Remote keys involved into transactions. Doesn't include near keys. */
+    /** Remote keys involved into transactions. Doesn't include near keys. A set: the sender dedups them. */
+    @Order(2)
     @GridToStringInclude
-    @Marshalled("txKeysArr")
-    Set<IgniteTxKey> txKeys;
+    Collection<IgniteTxKey> txKeys;

Review Comment:
   We can't โ€” and it turned out to be the more interesting answer, so let me lay it out.
   
   The codegen does honour the declared type on the `@Order` path: `MessageSerializerGenerator:551` emits `set=true` for a `Set` field, and `DirectByteBufferStream:1659` then builds `U.newHashSet` instead of an `ArrayList`. The catch is *when*: the set is filled inside the read loop (`col.add(item)`, :1667), and at that moment `IgniteTxKey#hashCode()` โ†’ `KeyCacheObjectImpl#hashCode()` throws `CacheObjectNotResolvedException`, because the reader created the key as `new KeyCacheObjectImpl(null, bytes, -1)` (`CacheObjectBinaryProcessorImpl:1299`). The value is resolved later, in the generated marshaller's `unmarshal`.
   
   I verified it instead of reasoning about it: declaring the field `Set<IgniteTxKey>` compiles, but `TxDeadlockDetectionMessageMarshallingTest` then fails with `GridDirectParser: Failed to read message ... CacheObjectNotResolvedException` and the NIO session closed. With `Collection` it passes.
   
   That is exactly what the companion array in master was for โ€” the generated ELEMENTS code does `for (e : arr) { unmarshal(e); col.add(e); }`, i.e. hashing deferred until the cache objects are resolved. The same pattern is still visible in `TxLocksResponse` for `nearTxKeyLocks`: `put(k, v)` happens only after `MessageMarshalling.unmarshal(k, ...)`.
   
   So keeping `Set` in the declaration means keeping the companion array. The wire is byte-identical either way (`writeCollection` and `writeObjectArray` both emit size + elements, and the `set` flag never reaches the wire), so the only thing it buys is the type-level guarantee โ€” and nothing depends on it. The sole consumers are `IgniteTxManager#hasKeys` (plain iteration) and `TxDeadlockDetection#mapTxKeys` (iteration into per-node `HashSet`s and a `UniqueDeque`): no `contains()` over these keys, no collection `equals`, no receive-side dedup needed. The sender-side dedup stays and does real work โ€” `TxLocksRequest`'s constructor still takes a `Set`, and `TxLocksResponse#addKey` builds one, which matters because the same key can be added from two different transactions.
   
   Expanded the field comment to state the reason rather than just assert "a set", so the next reader doesn't have to ask.



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java:
##########
@@ -147,19 +145,7 @@ public GridNearGetRequest(
         this.futId = futId;
         this.miniId = miniId;
         this.ver = ver;
-
-        this.keys = new ArrayList<>(keys.size());
-
-        if (addReader)
-            readersFlags = new ArrayList<>(keys.size());
-
-        for (Map.Entry<KeyCacheObject, Boolean> entry : keys.entrySet()) {
-            this.keys.add(entry.getKey());
-
-            if (addReader)
-                readersFlags.add(entry.getValue());
-        }
-
+        this.keyMap = keys;

Review Comment:
   Done โ€” the parameter there is `keys`, so the qualifier distinguished nothing (unlike the neighbouring lines, where the names do collide).



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedTxPrepareRequest.java:
##########
@@ -235,23 +226,20 @@ public byte policy() {
     }
 
     /**
-     * Adds version to be verified on remote node.
-     *
-     * @param key Key for which version is verified.
-     * @param dhtVer DHT version to check.
+     * @param key Key whose DHT version is verified on the remote node.
      */
-    public void addDhtVersion(IgniteTxKey key, @Nullable GridCacheVersion dhtVer) {
-        if (dhtVers == null)
-            dhtVers = new HashMap<>();
+    public void addDhtVersionKey(IgniteTxKey key) {
+        if (dhtVerKeys == null)
+            dhtVerKeys = new ArrayList<>();
 
-        dhtVers.put(key, dhtVer);
+        dhtVerKeys.add(key);
     }
 
     /**
-     * @return Map of versions to be verified.
+     * @return Keys whose DHT version is verified.
      */
-    public Map<IgniteTxKey, GridCacheVersion> dhtVersions() {
-        return dhtVers == null ? Collections.emptyMap() : dhtVers;
+    public Collection<IgniteTxKey> dhtVersionKeys() {
+        return dhtVerKeys == null ? Collections.emptyList() : dhtVerKeys;

Review Comment:
   Applied, thanks โ€” `Collections` import dropped with 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-28356 Get rid of redundant wire companion fields in cache messages [ignite]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5110836580

   <!-- ignite-rolling-upgrade-check -->
   ## Possible compatibility issues. Please, check rolling upgrade cases
   
   This PR modifies protected classes (with **Order** annotation).
   Changes to these classes can break rolling upgrade compatibility.
   
   **Affected files:**
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/GenericValueMessage.java`
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryStartRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedTxPrepareRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/KeyedVersionedValue.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/service/LazyServiceConfigurationMessage.java`
   


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3683125548


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridNearAtomicFullUpdateRequest.java:
##########
@@ -387,7 +379,7 @@ else if (conflictVers != null)
 
             forceDeploymentInfo(ctx);
 
-            if (entryProcessorsBytes == null)
+            if (entryProcessors != null)

Review Comment:
   The two cases are inverted, which is why the check moved.
   
   `entryProcessors` here is now `@Marshalled("entryProcessorsBytes")`, and the generated unmarshal nulls the companion once it has inflated it โ€” `GridNearAtomicFullUpdateRequestMarshaller:138`, `msg.entryProcessorsBytes = null;`. So under `@Marshalled` `entryProcessorsBytes == null` means "already unmarshalled", the opposite of what it used to mean.
   
   In `GridDhtAtomicUpdateRequest` the field is not `@Marshalled` (untouched by this PR): its hand-written unmarshal keeps the bytes, so there `entryProcessorsBytes == null` still means "not marshalled yet" and the check is right. Same for the `invokeArgsBytes == null` check one line below in this very method โ€” `invokeArgs` stayed on hand-written marshalling, so its check stayed too. The bytes check was dropped exactly where the field moved to codegen, not everywhere.
   
   Behaviour is identical either way: `deploy()` runs only on the send path (`GridCacheIoManager#onSend:1125`, plus two pre-send spots in `IgniteTxManager`), and on the sending node the companion is always null at that point, because the only writer is the generated marshal, which runs later in `GridIoManager#marshal`. Both spellings pick the same branch; the new one just doesn't become vacuously true on a received message.
   
   Added a one-line comment stating that reason.



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5120482855

   /run-all


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5117147779

   <!-- ignite-rolling-upgrade-check -->
   ## Possible compatibility issues. Please, check rolling upgrade cases
   
   This PR modifies protected classes (with **Order** annotation).
   Changes to these classes can break rolling upgrade compatibility.
   
   **Affected files:**
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/GenericValueMessage.java`
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryStartRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedTxPrepareRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridNearAtomicFullUpdateRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/KeyedVersionedValue.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/continuous/CacheContinuousQueryEntry.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/service/LazyServiceConfigurationMessage.java`
   


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3680981046


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java:
##########
@@ -147,19 +145,7 @@ public GridNearGetRequest(
         this.futId = futId;
         this.miniId = miniId;
         this.ver = ver;
-
-        this.keys = new ArrayList<>(keys.size());
-
-        if (addReader)
-            readersFlags = new ArrayList<>(keys.size());
-
-        for (Map.Entry<KeyCacheObject, Boolean> entry : keys.entrySet()) {
-            this.keys.add(entry.getKey());
-
-            if (addReader)
-                readersFlags.add(entry.getValue());
-        }
-
+        this.keyMap = keys;

Review Comment:
   Remove unnecessary 'this' qualifier.



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3681535031


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java:
##########
@@ -35,14 +34,9 @@ public class TxLocksRequest extends GridCacheMessage {
     long futId;
 
     /** Tx keys. */
-    @GridToStringInclude
-    @Marshalled("txKeysArr")
-    Set<IgniteTxKey> txKeys;
-
-    /** Array of txKeys from {@link #txKeys}. Used during marshalling and unmarshalling. */
-    @GridToStringExclude
     @Order(1)
-    IgniteTxKey[] txKeysArr;
+    @GridToStringInclude
+    Collection<IgniteTxKey> txKeys;

Review Comment:
   Why can't we keep Set?



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3681647834


##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareDestroyedCacheTest.java:
##########
@@ -163,13 +163,13 @@ public void testPrepareOfDestroyedCacheAnsweredWithErrorResponse() throws Except
 
         Throwable err = ((GridNearTxPrepareResponse)resps.get(0)).error();
 
-        assertNotNull("The response must carry the unmarshalling error.", err);
+        assertNotNull("The response must carry the error of the destroyed cache.", err);
 
-        CacheObjectNotResolvedException cause = X.cause(err, CacheObjectNotResolvedException.class);
+        CacheInvalidStateException cause = X.cause(err, CacheInvalidStateException.class);

Review Comment:
   Need to update javadoc for GridNearTxPrepareDestroyedCacheTest regarding 'throws {@link CacheObjectNotResolvedException}'.



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5131792972

   <!-- ignite-rolling-upgrade-check -->
   ## Possible compatibility issues. Please, check rolling upgrade cases
   
   This PR modifies protected classes (with **Order** annotation).
   Changes to these classes can break rolling upgrade compatibility.
   
   **Affected files:**
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/GenericValueMessage.java`
   - `modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryStartRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedTxPrepareRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridNearAtomicFullUpdateRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/KeyedVersionedValue.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/continuous/CacheContinuousQueryEntry.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/service/LazyServiceConfigurationMessage.java`
   


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3676496760


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridNearAtomicFullUpdateRequest.java:
##########
@@ -387,7 +379,7 @@ else if (conflictVers != null)
 
             forceDeploymentInfo(ctx);
 
-            if (entryProcessorsBytes == null)
+            if (entryProcessors != null)

Review Comment:
   Why the change? For example, in `GridDhtAtomicUpdateRequest#deploy` bytes are still checked for null before deployCollection is called.



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3681427845


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedTxPrepareRequest.java:
##########
@@ -235,23 +226,20 @@ public byte policy() {
     }
 
     /**
-     * Adds version to be verified on remote node.
-     *
-     * @param key Key for which version is verified.
-     * @param dhtVer DHT version to check.
+     * @param key Key whose DHT version is verified on the remote node.
      */
-    public void addDhtVersion(IgniteTxKey key, @Nullable GridCacheVersion dhtVer) {
-        if (dhtVers == null)
-            dhtVers = new HashMap<>();
+    public void addDhtVersionKey(IgniteTxKey key) {
+        if (dhtVerKeys == null)
+            dhtVerKeys = new ArrayList<>();
 
-        dhtVers.put(key, dhtVer);
+        dhtVerKeys.add(key);
     }
 
     /**
-     * @return Map of versions to be verified.
+     * @return Keys whose DHT version is verified.
      */
-    public Map<IgniteTxKey, GridCacheVersion> dhtVersions() {
-        return dhtVers == null ? Collections.emptyMap() : dhtVers;
+    public Collection<IgniteTxKey> dhtVersionKeys() {
+        return dhtVerKeys == null ? Collections.emptyList() : dhtVerKeys;

Review Comment:
   ```suggestion
           return F.emptyIfNull(dhtVerKeys);
   ```



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5134163697

   **[Ignite PR Checker](https://ignite-pr-checker.is-a.dev/?pr=13408)** verdict ยท RunAll build [9240744](https://ci2.ignite.apache.org/build/9240744) ยท 147 suites ran, 0 reused
   
   โš ๏ธ **1 broken suite(s)** (no reliable run):
   - Snapshots 6: JVM crash / out of memory ยท 1 failed test detected
   
   โœ… No test blockers otherwise; 28 pre-existing/flaky filtered out.
   
   โณ _Auto re-run **#1** in progress โ€” 1 broken suite(s) re-queued (attempt 1/2), **โ‰ˆ settled by 20:58 MSK**. This comment updates when they settle._


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov merged PR #13408:
URL: https://github.com/apache/ignite/pull/13408


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5116846141

   /runall


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3681647834


##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareDestroyedCacheTest.java:
##########
@@ -163,13 +163,13 @@ public void testPrepareOfDestroyedCacheAnsweredWithErrorResponse() throws Except
 
         Throwable err = ((GridNearTxPrepareResponse)resps.get(0)).error();
 
-        assertNotNull("The response must carry the unmarshalling error.", err);
+        assertNotNull("The response must carry the error of the destroyed cache.", err);
 
-        CacheObjectNotResolvedException cause = X.cause(err, CacheObjectNotResolvedException.class);
+        CacheInvalidStateException cause = X.cause(err, CacheInvalidStateException.class);

Review Comment:
   Need to update javadoc for GridNearTxPrepareDestroyedCacheTest regarding `throws {@link CacheObjectNotResolvedException}`.



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5121519820

   **[Ignite PR Checker](https://ignite-pr-checker.is-a.dev/?pr=13408)** verdict ยท RunAll build [9237865](https://ci2.ignite.apache.org/build/9237865) ยท 147 suites ran, 0 reused
   
   โœ… **No blockers** โ€” nothing in this run looks caused by this PR. 31 pre-existing/flaky tests filtered out.


-- 
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-28356 Remove hand-written wire conversion from TxLocksRequest and GridNearGetRequest [ignite]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5110256157

   <!-- ignite-rolling-upgrade-check -->
   ## Possible compatibility issues. Please, check rolling upgrade cases
   
   This PR modifies protected classes (with **Order** annotation).
   Changes to these classes can break rolling upgrade compatibility.
   
   **Affected files:**
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedTxPrepareRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareResponse.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/KeyedVersionedValue.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksResponse.java`
   


-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3683131336


##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxPrepareDestroyedCacheTest.java:
##########
@@ -163,13 +163,13 @@ public void testPrepareOfDestroyedCacheAnsweredWithErrorResponse() throws Except
 
         Throwable err = ((GridNearTxPrepareResponse)resps.get(0)).error();
 
-        assertNotNull("The response must carry the unmarshalling error.", err);
+        assertNotNull("The response must carry the error of the destroyed cache.", err);
 
-        CacheObjectNotResolvedException cause = X.cause(err, CacheObjectNotResolvedException.class);
+        CacheInvalidStateException cause = X.cause(err, CacheInvalidStateException.class);

Review Comment:
   Good catch โ€” the class javadoc described exactly the mechanism this PR removes. Rewrote it around the actual behaviour: the entry has no cache context to initialize against, so the prepare fails with `CacheInvalidStateException`. The `{@link}` and the import now point there.



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #13408:
URL: https://github.com/apache/ignite/pull/13408#discussion_r3681696289


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksResponse.java:
##########
@@ -43,21 +43,16 @@ public class TxLocksResponse extends GridCacheMessage {
     @Marshalled(keys = "nearTxKeysArr", values = "locksArr")
     final Map<IgniteTxKey, List<TxLock>> nearTxKeyLocks = new HashMap<>();
 
-    /** Remote keys involved into transactions. Doesn't include near keys. */
+    /** Remote keys involved into transactions. Doesn't include near keys. A set: the sender dedups them. */
+    @Order(2)
     @GridToStringInclude
-    @Marshalled("txKeysArr")
-    Set<IgniteTxKey> txKeys;
+    Collection<IgniteTxKey> txKeys;

Review Comment:
   Why can't we keep Set?



-- 
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-28356 Get rid of redundant wire companion fields and hand-written marshalling in messages [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.
anton-vinogradov commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5131606446

   Not done, and I believe it shouldn't be โ€” but I checked rather than assumed, so here is the evidence.
   
   `@UseBinaryMarshaller` is not free-standing: `CoreMessagesProvider` asserts it against the registration call โ€” `withSchema` requires it (`"Add @UseBinaryMarshaller for class: ..."`) and `withNoSchema` requires its absence (`"Remove @UseBinaryMarshaller for class: ..."`). `KeyedVersionedValue` is registered `withNoSchema` right next to its parent `CacheVersionedValue` (`CoreMessagesProvider:375-376`), and the parent carries no annotation either.
   
   I tried adding it: it compiles, but every node start then trips `java.lang.AssertionError: Remove @UseBinaryMarshaller for class: KeyedVersionedValue` โ€” `GridCacheNearReadersSelfTest` fails on all 7 methods. Reverted; the suite is green again.
   
   The reason it isn't needed is that no `Marshaller` ever reaches this class. It has no `@Marshalled` field and does not implement `MarshallableMessage`, so the generated `KeyedVersionedValueMarshaller` has no `Marshaller` field and no constructor taking one โ€” it only does `msg.val.marshal(ctx)` and `msg.key.marshal(ctx)`, i.e. the cache-object path driven by `CacheObjectContext`. The user data inside the key and the value is handled there, not by a `Marshaller`.
   
   If the intent is that this message should move to `withSchema` + the annotation on substance (it does carry user data, just through `CacheObject`), that would apply equally to `CacheVersionedValue` and to the other cache-object-only messages, and it would be a change of which marshaller the whole family gets โ€” worth its own ticket rather than a one-line addition here. Happy to file it if you see it that way.


-- 
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-28356 Remove hand-written wire conversion from TxLocksRequest and GridNearGetRequest [ignite]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13408:
URL: https://github.com/apache/ignite/pull/13408#issuecomment-5109177745

   <!-- ignite-rolling-upgrade-check -->
   ## Possible compatibility issues. Please, check rolling upgrade cases
   
   This PR modifies protected classes (with **Order** annotation).
   Changes to these classes can break rolling upgrade compatibility.
   
   **Affected files:**
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearGetRequest.java`
   - `modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java`
   


-- 
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