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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
