IGNITE-28940 Choose the marshaller by transport, not by message class - #13462
IGNITE-28940 Choose the marshaller by transport, not by message class#13462anton-vinogradov wants to merge 17 commits into
Conversation
The marshaller was picked from the message: @UseBinaryMarshaller on the class decided it, and the factory bound the chosen one into the generated companion at registration. But binary cannot be used where marshalling cannot afford a cluster-wide class registration, and that is a property of the call site, not of the message: registering waits for discovery, so a discovery thread must never do it. The marshaller is now a parameter of MessageMarshaller and MessageMarshalling, passed down to nested messages by the generated code. Communication call sites pass ctx.marshaller(), discovery call sites pass the jdk one. The annotation, the two marshaller fields of the factory provider and its init() are gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A message instance that travels both transports must not carry bytes of one into the other: BinaryMetadataVersionInfo lives in the local metadata cache, goes to discovery in the data bag and to communication in MetadataResponseMessage, and a marshalled instance keeps its serialized form. The response now stores a copy. GridTestUtils.loadMarshaller still asked the generated companion for a constructor taking a Marshaller, which the generator no longer writes, and IncrementalSnapshotTest injected an ErrorMessage marshalled with jdk into a communication message the receiver now reads with binary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t on a discovery thread Marshalling is done by the sending thread, and a schema-aware marshaller registers unknown class names in the cluster, which takes a discovery round. On a discovery thread that round never completes - this very thread is the one to deliver the answer. ErrorMessage carries an arbitrary user class and travels both transports, so it always uses the JDK marshaller of the local node. DistributedProcess sends the result of a process from a discovery thread whenever the process finishes synchronously, so it now hands the send over to the system pool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… send over to the pool A coordinator change between the handover and the actual send made the deferred task and the resend of the node left listener target the same new coordinator, so the result was sent twice. Resolving the coordinator upfront keeps the former addressing: the deferred send goes to the failed coordinator and is dropped, and the resend stays the only live one. The local coordinator branch marshals nothing, so it no longer goes through the pool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ts marshaller Every caller had to know which marshaller its transport speaks and pass it next to the kernal context the marshaller came from. Two entry points now name the transport itself: CommunicationMarshalling for the schema-aware one and DiscoveryMarshalling for the JDK one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oints The classes are final and hold static methods only, so nothing can instantiate them anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nsports The wire form of a @Marshalled field is cached in its companion field, so an instance marshalled by one transport hands the other transport bytes of a format it does not read. A shared instance crosses transports in DistributedProcess, which sends the result of a process by communication and then by discovery, and in the binary metadata cache, whose entries answer requests and travel the data bag at once. The new @JdkMarshalled tells the generated marshaller to use the JDK marshaller of the local node whatever the transport speaks, so the cached bytes stay readable by both. It replaces what these classes did by hand: the private helper of ErrorMessage and the defensive copy of MetadataResponseMessage, both dropped. PartitionHashRecord kept the cached bytes across java serialization, since its byte fields were not transient unlike the ones of its neighbours. DistributedProcessMarshalThreadTest covers the guard that keeps a result off the discovery thread. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng it off the discovery thread The result travels both transports, so the marshaller has to be the same on both legs and it has to be the one that needs no class name registration. @JdkMarshalled on SingleNodeMessage states exactly that, and it covers every payload of every process, including the ones not written yet. That makes the discovery thread guard of sendSingleMessage pointless: marshalling the result no longer waits for a discovery round, so it is safe where master has always done it. The method is back to its former shape, and with it goes the handover to the pool that reordered the send against the node left resend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/runall |
|
/top |
|
/runall 🚀 RunAll queued — build 9275680 · live progress & verdict: Ignite PR Checker. The verdict lands here when the run finishes. |
|
@anton-vinogradov 🚀 RunAll queued — build 9275976 · live progress & verdict: Ignite PR Checker. Your previous run was cancelled — this one supersedes it. The verdict lands here when the run finishes. |
Ignite asserts that every test class belongs to a suite, and the check runs before the test chain, so the whole RunAll stopped at it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/run-all |
|
@anton-vinogradov that looks like an Ignite PR Checker command — but the checker doesn't know your accounts yet, so nothing was triggered. Everything it does runs under your own accounts (there is no bot); setting that up takes about two minutes:
Then comment here:
Your command comment gets a 🚀 and narrates the run — live ETA, finish, auto re-run waves — and the verdict lands as one comment that updates in place until everything settles. Tokens are stored encrypted, and only while the options are on. |
|
/top |
1 similar comment
|
/top |
|
/runall 🚀 RunAll queued — build 9278217 · live progress & verdict: Ignite PR Checker. Your previous run was cancelled — this one supersedes it. The verdict lands here when the run finishes. |
Extracting the send was only needed to hand it to the pool from the discovery thread guard, which is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rshaller in unmarshalNio The pin belongs to the message, so a subclass has to marshal the inherited fields the same way its parent does. It did not: CalciteErrorMessage took the marshaller of the transport while ErrorMessage took the JDK one, which sent an exception through binary on communication where master sent it through jdk. The nio method emitted the name of the pinned marshaller without declaring it, so a message that combined @JdkMarshalled with @NioField did not compile at all. The codegen test now covers both: the fixture carries a @NioField, and a subclass of it checks that the pin reaches the generated companion of a child. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| @Documented | ||
| @Target(value = TYPE) | ||
| @Retention(RUNTIME) | ||
| public @interface UseBinaryMarshaller { |
There was a problem hiding this comment.
Why binary changed on opposite JDK?
There was a problem hiding this comment.
Now Discovery uses jdk, Communication - binary, with some exceptions.
| * Marshalling of the discovery transport, which speaks the JDK marshaller: a discovery message is marshalled on a | ||
| * discovery thread, where waiting for a cluster-wide type registration would never finish. | ||
| */ | ||
| public final class DiscoveryMarshalling { |
There was a problem hiding this comment.
Why it is in communication package? Also package managers might be revised
There was a problem hiding this comment.
Agree, the package does not fit them. Moving these two means moving MessageMarshalling as well, since they sit next to it and it comes from master, so I would rather do all three in a separate issue than widen this one.
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| /** Marshalling of the communication transport, which speaks the schema-aware marshaller. */ | ||
| public final class CommunicationMarshalling { |
There was a problem hiding this comment.
Only minority of the messages requires BinaryMarshaller. Why we use it for all of them? It is able to marshal non-serializable and won't fail where it should. Is it faster than JDKMarshaller? Any proofs? It processes schema. Won't we get any perf. drop on marshalling with it?
There was a problem hiding this comment.
Measured it. Binary is faster on every payload a message field actually carries, except exceptions, where the two are equal.
Round trip (marshal + unmarshal), corretto-17, 2000 warmup + 20000 iterations, three runs, numbers from the last one:
| payload | jdk, us/op | binary, us/op | speedup | jdk, bytes | binary, bytes |
|---|---|---|---|---|---|
| small POJO | 6.21 | 3.43 | 1.8x | 121 | 40 |
| list of 100 POJOs | 57.72 | 22.24 | 2.6x | 2048 | 4196 |
| map of 50 String to UUID | 14.95 | 5.38 | 2.8x | 1626 | 1346 |
| exception with a cause | 20.59 | 24.79 | 0.83x | 2138 | 2144 |
| CacheConfiguration | 27.60 | 7.60 | 3.6x | 3432 | 636 |
| QueryEntity | 7.74 | 1.48 | 5.2x | 783 | 161 |
Two results are worth reading carefully.
Exceptions are a tie, and that is expected. Throwable declares writeObject, so BinaryUtils.isCustomJavaSerialization sends it to OptimizedMarshaller - the binary format never runs. That is also why ErrorMessage pins jdk in this PR: binary buys nothing there and would only register the class name of every new exception type cluster-wide.
Binary is bigger on a list of equal objects. It writes the schema per object, while jdk writes the class descriptor once and back-references it. On everything else binary is smaller, up to 5x on CacheConfiguration.
The schema processing you asked about is what pays for itself here: it is written per object, but reading does not walk the whole graph.
The benchmark is a throwaway test class, not part of the PR. It builds both marshallers the way tests do - Marshallers.jdk() and createStandaloneBinaryMarshaller() - and times U.unmarshal(marsh, U.marshal(marsh, obj), loader) in a loop. I can attach it if you want to re-run it.
| } | ||
|
|
||
| MessageMarshalling.unmarshal(e, ctx, cctx.cacheObjectContext(), ldr); | ||
| CommunicationMarshalling.unmarshal(e, ctx, cctx.cacheObjectContext(), ldr); |
There was a problem hiding this comment.
The same. Why Communication marshaling? How do I know it? To me, MessageMarshalling look much better. It will decide what and how to do. Now I need to decide. Let's reconsider.
There was a problem hiding this comment.
Let's refactor naming, linking to the thread in a separate issue.
This one has another goal.
|
|
||
| try { | ||
| MessageMarshalling.unmarshal(cacheMsg, cctx.kernalContext(), null, cctx.deploy().globalLoader()); | ||
| CommunicationMarshalling.unmarshal(cacheMsg, cctx.kernalContext(), null, cctx.deploy().globalLoader()); |
There was a problem hiding this comment.
Same. Why CommunicationMarshalling ? Why if someone puts DiscoveryMarshalling? Is it deprecated, protected?
There was a problem hiding this comment.
Tests will fail :)
| * All changes must be made with the respect of RU rules. | ||
| */ | ||
| // Travels both transports: by discovery when a cache starts, by communication when a snapshot is restored. | ||
| @JdkMarshalled |
There was a problem hiding this comment.
Even so, why not marshall with the marshaller choosen? Why discovery ort Communication should change marshaller?
There was a problem hiding this comment.
The not changing marshallers, they keep it as is with minor fixes, which was luckily passed before.
|
|
||
| try { | ||
| MessageMarshalling.unmarshal(req, ctx, null, U.resolveClassLoader(clsLdr, ctx.config())); | ||
| CommunicationMarshalling.unmarshal(req, ctx, null, U.resolveClassLoader(clsLdr, ctx.config())); |
There was a problem hiding this comment.
Same. MessageMarshalling looks better. Noone cares which marshalling to call. Just pick up MessageMarshalling. Now we need to know.
| boolean loc = ctx.localNodeId().equals(res.nodeId()) && !ctx.config().isMarshalLocalJobs(); | ||
|
|
||
| if (!loc) | ||
| MessageMarshalling.unmarshal(res, ctx, null, U.resolveClassLoader(dep.classLoader(), ctx.config())); |
There was a problem hiding this comment.
All the others places too. MessageMarshalling looks better. Gets the work of marsahaling type choise incapsulated. Not, exposed to a developer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Possible compatibility issues. Please, check rolling upgrade casesThis PR modifies protected classes (with Order annotation). Affected files:
|
|
Ignite PR Checker verdict · RunAll build 9278815 · 147 suites ran, 0 reused
Everything below is what it did manage to say. 🔎 No blockers found — but the run above can't prove the PR is clean. 22 pre-existing/flaky tests filtered out. Re-run once the above is sorted out. ♻️ Settled after 2 auto re-run wave(s): #1 — early: Control Utility 1; #2 — 1 broken suite(s). |
|
Ignite PR Checker verdict · RunAll build 9279430 · 147 suites ran, 0 reused ✅ No blockers — nothing in this run looks caused by this PR. 20 pre-existing/flaky tests filtered out. ♻️ Settled after 1 auto re-run wave(s): #1 — 2 watch suite(s). |
| try { | ||
| return loadMarshaller(msg); | ||
| } | ||
| catch (RuntimeException ignored) { |
There was a problem hiding this comment.
Looks too tough. Maybe we need a comment here?
| // Marshal eagerly: the heavy partition-map copy lands in the "Full message preparing" stage, and the | ||
| // message cached in FinishState is sent to late joiners as is (the send-path marshal-once turns no-op). | ||
| MessageMarshalling.marshal(msg, cctx.kernalContext(), null); | ||
| CommunicationMarshalling.marshal(msg, cctx.kernalContext(), null); |
There was a problem hiding this comment.
Refactoring this issue might set us to revert or rewrite this ticket again I'm afraid. We split marshalling type. This is the nase of this ticket
| * goes to the coordinator by communication and comes back in the {@link FullMessage} by discovery, while a marshalled | ||
| * field caches its wire form for the second leg. | ||
| */ | ||
| public class DistributedProcessResultMarshallingTest extends GridCommonAbstractTest { |
There was a problem hiding this comment.
One Test for one @Test? Can we put it in some existing Test?
2 participants
The marshaller used for a message's
@Marshalledfields was a property of themessage class:
@UseBinaryMarshallerdecided it, and the factory bound thechosen marshaller into the generated companion at registration time.
Why that is the wrong place
Binary cannot be used where marshalling cannot afford a cluster-wide class
registration, and that is a property of the call site. The registration is
MarshallerContextImpl#registerClassName->proposeMapping->fut.get(),which waits for discovery, so it must never happen on a discovery thread. Ignite
classes usually skip it thanks to
META-INF/classnames.properties, but that listis not closed under nesting - a plain
CacheConfigurationneeds types that arenot on it.
The call sites are already separated by transport, and the hand-written half of
the API works this way:
MarshallableMessage#marshal(Marshaller)takes themarshaller as a parameter. Only generated code kept it in a field.
Change
Marshalleris now a parameter ofMessageMarshallerandMessageMarshalling;the generator stops storing it and passes it down to nested messages.
ctx.marshaller(), discovery call sites passmarshallerContext().jdkMarshaller().@UseBinaryMarshaller(60 classes),AbstractMessageFactoryProvider#initandboth marshaller fields are gone, as is
initProviderinIgniteKernal.Wire format
The format is now a function of the transport, not of the class, so a class that
travels both transports is marshalled differently on each. Deliberate changes:
ErrorMessage,PartitionHashRecord,TransactionsHashRecordBinaryMetadataVersionInfoStoredCacheDataStoredCacheDatais not in the ticket's list: it was found while reviewing thischange. It carries
CacheConfigurationandQueryEntityblobs and reachescommunication through
SnapshotRestoreOperationResponse->SingleNodeMessage,so the transport rule moves it to binary. Registration is allowed there, and
binary is the smaller of the two for a
CacheConfiguration.BinaryMetadataVersionInfoneeded one more fix: the same long-lived instancefrom the local metadata cache travels discovery in the data bag and communication
in
MetadataResponseMessage, and a marshalled instance keeps its serializedform.
MetadataResponseMessage#metadataVersionInfonow stores a copy, so bytesof one transport cannot leak into the other.
Verified
MessageProcessorTest(codegen goldens regenerated),IgniteCoreMessagesSerializationTest,DirectMarshallingMessagesTest,MessageMarshalOnceTest,CompressedMessageTest,GridIoManagerOrderedUnmarshalFailureTest,DiscoveryUnmarshalVulnerabilityTest,QueryEntityMessageSerializationTest,LazyServiceConfigurationMessageSerializationTest,SecurityBasicPermissionSetSerializationTest,TxDeadlockDetectionMessageMarshallingTest,GridCacheQueryResponseUnmarshalTest,BinaryMetadataRegistrationInsideEntryProcessorTest,GridCacheAtomicFullApiSelfTest, the continuous-query and p2p suites, and thecalcite
PlanExecutionTest/ContinuousExecutionTest/CalciteCommunicationMessageSerializationTest.Full build of all modules and the strict checkstyle profile are clean.
🤖 Generated with Claude Code