IGNITE-28520 Auto-generate message serialization / marshalling / deployment#13095
IGNITE-28520 Auto-generate message serialization / marshalling / deployment#13095anton-vinogradov wants to merge 279 commits into
Conversation
anton-vinogradov
force-pushed
the
ignite-28520
branch
from
cb7ec5a to
802f7e1
Compare
# Conflicts: # modules/codegen/src/main/java/org/apache/ignite/internal/MessageSerializerGenerator.java # modules/commons/src/main/java/org/apache/ignite/internal/processors/cache/version/GridCacheVersion.java # modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheReturn.java # modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/operation/SchemaAddQueryEntityOperation.java # modules/core/src/test/java/org/apache/ignite/internal/direct/stream/DirectByteBufferStreamImplByteOrderSelfTest.java # modules/core/src/test/resources/codegen/TestCollectionsMessageSerializer.java # modules/core/src/test/resources/codegen/TestMapMessageSerializer.java # modules/core/src/test/resources/codegen/TestMessageSerializer.java
| package org.apache.ignite.plugin.extensions.communication; | ||
|
|
||
| /** Marker interface: no {@link MessageMarshaller} is generated for implementing classes. */ | ||
| public interface NonMarshallableMessage extends Message { |
There was a problem hiding this comment.
Should we use an annotation instead of an empty interface. Empty interface is a bad, outdated practice to me.
There was a problem hiding this comment.
An annotation can't give us two things this marker relies on: (1) NonMarshallableMessage extends Message, so the compiler itself guarantees the marker is only placed on messages — with an annotation we'd need another processor validation for that; (2) interface inheritance is exactly the semantic we need — the generator skips marshal recursion for a field of type X because any subtype of X is also non-marshallable, which plain annotations don't provide (@Inherited is subtle both at runtime and in annotation processing). Plus the runtime side (isAssignableFrom at registration, instanceof in the factory guarantees) stays trivial, and it follows the codebase convention for message markers (GridCacheDeployable, IgniteTxStateAware, …).
| /** Marks a {@code Collection} field whose wire form is a companion {@code @Order} array field named by {@link #value()}. */ | ||
| @Retention(RetentionPolicy.CLASS) | ||
| @Target(ElementType.FIELD) | ||
| public @interface MarshalledCollection { |
There was a problem hiding this comment.
Is it so necessary to bring one more annotation? Can we use Marshalled?
There was a problem hiding this comment.
They intentionally encode four different mechanics, not one: @Marshalled = a single Marshaller blob for an arbitrary object; @MarshalledCollection/@MarshalledMap = per-element Message serialization (no Marshaller involved at all; the map flavour needs two companion fields, keys()/values()); @MarshalledObjects = per-element Marshaller blobs so each element keeps its own deployment class loader (P2P) and query-row keys get finished. Folding them into one annotation would mean a union of attributes where half the combinations are invalid (validation moves into processor errors), and the field declaration would stop telling the reader which of the four mechanics applies.
| this.txHashRes = txHashRes; | ||
| this.partHashRes = partHashRes; | ||
| this.partiallyCommittedTxs = partiallyCommittedTxs; | ||
| this.exceptions = exceptions == null ? null : F.viewReadOnly(exceptions, ErrorMessage::new); |
There was a problem hiding this comment.
viewReadOnly is a lazy view that creates fresh ErrorMessage instances on every iteration. The flow is two-pass — MessageMarshaller.marshal fills each element's serialized form, then writeTo iterates again — so with a lazy view the write pass would see brand-new elements with the marshalled state lost. Now switched to eager F.transform, which keeps the one-liner while materializing the collection (same fix applies in GridDhtPartitionsFullMessage and ServiceSingleNodeDeploymentResult).
| */ | ||
| public abstract class MessageGenerator { | ||
| /** Blank separator line in generated code. */ | ||
| public static final String EMPTY = ""; |
There was a problem hiding this comment.
Because the accumulated entries are terminator-free lines: buildClassCode writes each as line + NL. A separator entry therefore has to be the empty string — using NL would render as a double blank line.
| MessageGenerator(ProcessingEnvironment env) { | ||
| this.env = env; | ||
|
|
||
| cacheIdAwareMirror = type("org.apache.ignite.plugin.extensions.communication.CacheIdAware"); |
There was a problem hiding this comment.
org.apache.ignite.plugin.extensions.communication.CacheIdAware might be constant
There was a problem hiding this comment.
Can't be a Class reference — CacheIdAware lives in core, which isn't on the codegen classpath (that's exactly why external types are looked up as FQN-string mirrors). And per the sibling thread in MessageDeploymentGenerator we've just unified all single-use type FQNs to plain strings, so a String constant would go against that.
| " */"; | ||
|
|
||
| /** */ | ||
| final ProcessingEnvironment env; |
There was a problem hiding this comment.
protected? The others fields and methods below too,
There was a problem hiding this comment.
Visibility here is "tightest that works": public only for what's genuinely used from another package (NL/TAB and the static helpers are imported by the internal.idto processors), package-private for everything the subclass generators and the processor need — they all share this package. protected would only widen access to hypothetical out-of-package subclasses.
| package org.apache.ignite.plugin.extensions.communication; | ||
|
|
||
| /** | ||
| * Implemented by messages targeting a specific cache. The cache ID lets the generated marshaller and deployer resolve |
There was a problem hiding this comment.
targeting a specific cache -> smth. like holding certain cache data/CacheObject to transfer?
There was a problem hiding this comment.
Agree, reworded — now "Implemented by messages that carry cache data (CacheObjects) to transfer. The cache ID lets the generated marshaller and deployer resolve the cache object context used to (un)marshal and deploy that data."
| * the cache object context used to (un)marshal and deploy the message's cache objects. | ||
| */ | ||
| public interface CacheIdAware { | ||
| /** @return Cache ID identifying the target cache. */ |
There was a problem hiding this comment.
target cache -> smth. like cache data/CacheObject to transfer
There was a problem hiding this comment.
Done — now "ID of the cache whose data the message transfers."
| /** | ||
| * Implemented by messages targeting a specific cache. The cache ID lets the generated marshaller and deployer resolve | ||
| * the cache object context used to (un)marshal and deploy the message's cache objects. | ||
| */ |
There was a problem hiding this comment.
Wy might also refer some @see to a factory or a serializer.
There was a problem hiding this comment.
Added @see MessageMarshaller and @see MessageFactory.
|
|
||
| /** {@inheritDoc} */ | ||
| @Override public int hashCode() { | ||
| assert val != null; |
There was a problem hiding this comment.
It isn't, unfortunately: with assertions off (production) the control falls through to IgniteUtils.hashCode(null), which silently returns 0 — an unresolved key would quietly land in the wrong hash bucket instead of failing. The exception also isn't just diagnostics: GridCacheIoManager catches CacheObjectNotResolvedException next to BinaryObjectException and routes it to onClassError, i.e. the regular per-message error handling. The contract (query row keys travel bytes-only and must arrive resolved) is pinned by GridCacheQueryResponseUnmarshalTest.
|
|
||
| /** @return the serializer registered for {@code msg}'s direct type. */ | ||
| @SuppressWarnings("unchecked") | ||
| private static <M extends Message> MessageSerializer<M> resolve(MessageFactory factory, M msg) { |
There was a problem hiding this comment.
Is used only within the interface. Do we need to expose it? Or we need some SerializationUtil/Helper?
There was a problem hiding this comment.
It's actually called from a dozen production sites — GridNioServer, GridDirectParser, DirectByteBufferStream, TcpHandshakeExecutor, TcpDiscoveryIoSession, the ZK DiscoveryMessageParser (the private resolve below is the interface-internal one). Rationale for statics-on-interface — see the sibling thread above.
| * @param <M> Message type. | ||
| * @return Whether message was fully written. | ||
| */ | ||
| static <M extends Message> boolean writeTo(MessageFactory factory, M msg, MessageWriter writer) { |
There was a problem hiding this comment.
Do we need to expose it? Or we need some SerializationUtil/Helper or an abstract class instead of declaring static methods in an interface?
There was a problem hiding this comment.
They're the family-wide pattern: instance methods are the per-type generated implementations, while the interface statics are the resolve-and-dispatch entry point — same as MessageMarshaller.marshal/unmarshal and GridCacheMessageDeployer.deploy. A separate SerializationUtil would split the contract and its entry point across two types, and an abstract class isn't an option — this is an interface-based SPI implemented by the generated serializers. The statics are also the enforced entry: MessageSerializationArchitectureTest (ArchUnit) fails any direct instance-method call.
Possible compatibility issues. Please, check rolling upgrade casesThis PR modifies protected classes (with Order annotation). Affected files:
|
What
Extends the annotation-driven message codegen (previously serialization-only) to
marshalling and deployment: a message's wire read/write, marshalling and
deployment are now generated from declarative field annotations, replacing
hand-written writeTo/readFrom, prepareMarshal/finishUnmarshal and prepareDeployment.
Main directions
Generated marshaller & deployer companions.
MessageProcessornow emits<Msg>Marshallerand<Msg>Deployernext to<Msg>Serializer, driven by@Marshalled/@MarshalledCollection/@MarshalledMap(with@Order/@NioField). Hand-written marshalling/deployment hooks are removed from messages.Uniform factory dispatch.
MessageFactoryresolves all three by direct type —serializer()/marshaller()/deployer()— via oneregister(directType, supplier, serializer, marshaller, deployer). Callers use thestatic facades
MessageSerializer.writeTo/readFrom,MessageMarshaller.marshal/unmarshal,GridCacheMessageDeployer.deploy; an ArchUnit rule (MessageSerializationArchitectureTest)enforces it.
Comm & discovery wired to it.
GridIoManager/GridCacheIoManager/IgniteTxManagerand the TCP/ZK discovery I/O callMessageMarshaller.marshal/unmarshal;discovery custom messages (
MetadataUpdateProposedMessage,BinaryMetadataVersionInfo, …)move from
MarshallableMessagehooks to@Marshalled.Deployment collapsed.
GridCacheMessageDeployeris now only the codegen interfaceplus the factory-resolving facade; per-field static bridges dropped,
GridCacheMessage#deploy*accessed directly.
Marshalling stays off the NIO threads. The NIO codec does only serialization
(
writeTo/readFrom); the actual marshal/unmarshal runs on the sender and workerthreads.
@NioField+unmarshalNioare the explicit, minimal exception — only thehandful of fields needed for early dispatch are unmarshalled on the NIO thread.
Side effects (mechanical — skim)
prepareMarshal→marshal,finishUnmarshal→unmarshal,finishUnmarshalNio→unmarshalNio(onMarshallableMessage/MessageMarshaller);CacheObject#prepareMarshal(ctx)/finishUnmarshal(ctx)→marshal/unmarshal;prepareDeployment+prepare*Deploymenthelpers →deploy/deploy*;MarshallerCacheFreeFinishTest→…UnmarshalTest,MessageFinishUnmarshalOnceTest→MessageUnmarshalOnceTest..mvn/jvm.configopensjdk.compilerfor the codegen tests(Google compile-testing);
commit-check.ymlnow surfaces the real compile errorbehind the generated-class "cannot find symbol" cascade.
Performance
JMH-verified: serialization hot path unchanged (Δ ≈ 0 vs master); marshalling adds
~1.8 ns/message of factory dispatch (<0.5% of the actual
U.marshal, ~µs); zero extraallocations.