PakhomovAlexander commented on code in PR #7914:
URL: https://github.com/apache/ignite-3/pull/7914#discussion_r3032515056
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/SnapshotEntry.java:
##########
@@ -130,7 +129,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- return Objects.hash(version);
+ return version;
Review Comment:
**Hash code values change with this PR — please verify this is safe for all
affected classes.**
This is not a transparent refactor. `Objects.hash()` and the manual `31 *
result + ...` pattern produce **different hash codes** for the same inputs,
because `Objects.hash()` (which delegates to `Arrays.hashCode(Object[])`)
starts with an initial seed of `1`, while the manual pattern seeds with the
first field's hash directly.
Concretely:
**Single-field case** (this file, `NullableValue`,
`DisposableDeploymentUnit`, etc.):
- Before: `Objects.hash(version)` → `Arrays.hashCode(new Object[]{version})`
→ `31 * 1 + version` = **`31 + version`**
- After: `return version;` → just **`version`**
**Two-field case** (e.g., `StoredRaftNodeId`, `ClusterTag`, `PartitionKey`):
- Before: `Objects.hash(a, b)` → `31 * (31 * 1 + hash(a)) + hash(b)` =
**`961 + 31*hash(a) + hash(b)`**
- After: `31 * hash(a) + hash(b)`
**N-field case** — same pattern, the `31^N` constant from the initial seed
propagates through.
For purely in-memory `HashMap`/`HashSet` usage this is fine — the JVM makes
no guarantees about hash stability across versions anyway. But in a distributed
system like Ignite, it's worth explicitly verifying that **none of the affected
classes** have their `hashCode()` result:
1. **Persisted to disk** (e.g., as part of a serialized data structure,
snapshot, or index)
2. **Transmitted over the network** and compared/used on a remote node
(e.g., for partition assignment or routing)
3. **Used in rolling-upgrade scenarios** where nodes running old code and
new code must agree on hash values
4. **Stored in external systems** (e.g., logged and later grep'd, or used as
cache keys in an external store)
Classes I'd look at most carefully given their names and locations:
- `SnapshotEntry` (this file) — catalog storage, snapshot-related
- `Lease` — placement driver leases
- `PartitionKey` — partition replicator, raft snapshots
- `RaftGroupConfiguration` — raft consensus config
- `CatalogSerializationChecker` — serialization tests (the test itself may
need updated expected values)
If any of these persist or transmit hash codes, this change could cause
subtle issues during rolling upgrades (old nodes produce `31 + version`, new
nodes produce `version`).
If you've already verified this — a brief note in the PR description ("hash
codes are not persisted or transmitted for any of these classes") would help
future readers understand why the change is safe.
--
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]
