nizhikov commented on PR #13332:
URL: https://github.com/apache/ignite/pull/13332#issuecomment-4902738702
## Review — correctness
Reviewed the switch-based inline serialization of
`GridCacheVersion`/`GridCacheVersionEx` and `IgniteProductVersion`. The
implementation is **correct**.
### `DirectByteBufferStream.writeGridCacheVersion` / `readGridCacheVersion`
- Byte layout is symmetric between write and read:
- plain: `[type=1][int topVer][int nodeOrderDrId][long order]`;
- `Ex`: outer three fields (`type=2`) followed by the conflict version's
three fields.
- The read path writes the nested fields **directly into the attached
`drVer`** (`ver.conflictVersion().topologyVersion(...)`, etc.), so the conflict
version is populated in place — no detached/orphan object, and the `Ex` DR
version round-trips correctly.
- Each `case` increments `uuidState` before falling through, so
partial-buffer resume re-enters at the correct case. Since case 0 always sets
`cur = ver`, `uuidState > 0` always coincides with a non-null `cur`, so the
`cur != NULL ? (GridCacheVersion)cur : null` entry line can't yield a null
`ver` mid-read.
- Matches the existing house style in this file (`writeUuid` /
`writeIgniteUuid` use the same fall-through `switch (uuidState)` idiom).
### `IgniteProductVersion`
Same clean conversion to a fall-through switch; byte-symmetric, null handled
at case 0.
### Notes (non-blocking)
- The conflict version is (correctly) assumed to be plain — cases 4–6 handle
exactly one nesting level and never recurse, consistent with
`GridCacheVersionEx`'s invariant (`drVer != null && !(drVer instanceof
GridCacheVersionEx)`). A one-line comment stating this assumption would help
future readers, since the previous explicit "Support one nest level, only!"
guard is gone.
- The `default: throw new IllegalStateException(...)` in each switch is good
defensive coding.
### Testing
Unit coverage looks good: the `dataCenterId() != 0` fixture guard plus
asserting `conflictVersion()` equality **and**
`conflictVersion().dataCenterId()` covers the DR field that
`GridCacheVersion.equals()` masks off (`equals` only compares
`topVer`/`order`/`nodeOrder`, and `GridCacheVersionEx` doesn't override it — so
an equals-only assertion would silently pass on a wrong DR version).
Byte-by-byte fragmentation now exercises every partial-read boundary.
Two integration-level checks worth running before merge, outside these unit
tests:
1. **Rolling-upgrade / wire compatibility** — the on-wire layout of every
message carrying a `GridCacheVersion` field changed (previously full `Message`
framing, now headerless inline bytes). Please confirm this break is acceptable
for the target release or run the `compatibility` module.
2. **End-to-end message round-trip** carrying an `Ex` conflict version,
including a collection/map of `GridCacheVersion` (the new `GRID_CACHE_VERSION`
`MessageCollectionItemType` path in `writeCollection`/`readMap`).
--
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]
