nizhikovprivate commented on PR #13410:
URL: https://github.com/apache/ignite/pull/13410#issuecomment-5116931925
# Review: IGNITE-28929 — Move marshaller info into Messages classes
**State reviewed**: branch at `b8cf88d250f`, against `master`. 321 files,
~735 insertions.
**Verdict: LGTM with two optional hardening suggestions.** The design is
sound, the annotation sweep is exhaustively verified correct, and the one
blocker found during review has been fixed and verified at build and runtime
level.
## What the change does
Per the Jira ticket (IEP-132 Communication), marshaller selection moves from
registration-site arguments into the message classes themselves via two new
runtime annotations, `@UseJdkMarshaller` and `@UseBinaryMarshaller`
(`org.apache.ignite.internal`).
`AbstractMarshallableMessageFactoryProvider.register` resolves the marshaller
from the annotation (`dfltMarsh` vs `schemaAwareMarsh` slot) and throws
`IllegalStateException` if a class carries neither.
`CoreMessagesProvider.withSchema`/`withNoSchema` remain as assertion-backed
documentation of intent; the Calcite and ZK factories drop their explicit
marshaller arguments.
## Verified correct (mechanically, not sampled)
- **All 297 core registrations match**: every `withNoSchema` class carries
`@UseJdkMarshaller`, every `withSchema` class `@UseBinaryMarshaller` — zero
mismatches. Wire format is therefore unchanged for all core messages.
- **All 12 Calcite registrations match** master's explicit arguments
(`QueryStartRequest`, `GenericValueMessage` → binary; the other ten → JDK).
**All 6 ZK classes** correctly carry `@UseJdkMarshaller`.
- **Coverage is exact both directions**: 315 annotated classes = 315
registrations; no double-annotated class, no annotated-but-unregistered class,
no leftover caller of removed overloads.
- **Build**: full `mvn install` of core, zookeeper, and all upstream modules
passes.
- **Runtime**: a probe registering `ZkMessageFactory` into a real
`IgniteMessageFactoryImpl` and calling `create()` for ids 400–405 instantiates
all six ZK messages cleanly.
## Blocker — found, fixed, verified
The original commit routed `ZkMessageFactory` through the reflective
`register(factory, cls, id)` path. Two ZK message classes are package-private,
which breaks reflection from the provider's package (confirmed empirically):
1. `ZkNoServersMessage` — implicit package-private constructor →
`NoSuchMethodException` → **node startup failure** with ZK discovery.
2. `ZkCommunicationErrorResolveFinishMessage` — public ctor in non-public
class → `IllegalAccessException` on first `create(400)`, a latent crash in the
communication-error-resolve path.
The fix (in `b8cf88d250f`) restores a `protected register(factory, cls, id,
supplier)` overload and passes in-package `::new` references from
`ZkMessageFactory`. It improves on the pre-change design: the supplier overload
takes no marshaller argument — annotation resolution happens inside it, so the
annotation stays the single source of truth on both paths. The "use this
overload for package-private classes" javadoc now sits on the protected
overload where callers see it. Verified by inspection, module build, and the
runtime probe above.
## Remaining suggestions (non-blocking)
1. **Assert-only validation**: the both-annotations-present conflict check
in `AbstractMarshallableMessageFactoryProvider.register` and the
annotation-vs-`withSchema`/`withNoSchema` consistency checks in
`CoreMessagesProvider` are `assert`s, which vanish in production. A future
mis-annotation would then *silently change the wire format* instead of failing
fast — with assertions disabled, the `if/else-if` ordering quietly prefers JDK
on conflict. These run once at startup over ~300 classes, so hard
`IllegalStateException`s cost nothing.
2. **Annotation javadoc precision**:
`@UseJdkMarshaller`/`@UseBinaryMarshaller` hard-name concrete marshallers, but
they actually select the injected `dfltMarsh`/`schemaAwareMarsh` slots, which
plugins may override via `init()`. One clarifying sentence would prevent wrong
assumptions.
## Notes for the record
- The 3-arg overload's javadoc ("Register a message with a caller-provided
`id`") doesn't distinguish it from the 4-arg — its actual contract is
"instantiates reflectively via the public no-arg ctor; class must be public."
Cosmetic.
- Removing/privatizing the old `protected static register` overloads breaks
third-party subclasses compiled against them; acceptable for an `internal`
package, but worth a quick check that nothing in ignite-extensions extends this
class before merge.
- The whitespace-only hunks in `SessionChannelMessage`,
`DataStreamerResponse`, `TcpInverseConnectionResponseMessage` are harmless
cleanups; `@Target(value = TYPE)` vs `@Target(TYPE)` and a stray blank import
line in `UseJdkMarshaller.java` are style trivia.
- Suggest running the ZK discovery and Calcite suites on TC before merge —
the blocker showed those paths weren't exercised locally.
🤖 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
