IGNITE-28520 Auto-generate message serialization / marshalling / deployment - #13095
Conversation
cb7ec5a to
802f7e1
Compare
|
|
/runall 🚀 RunAll queued — build 9228148. The verdict lands here when the run finishes. |
|
Ignite PR Checker verdict · RunAll build 9228148 · 147 suites ran, 0 reused ❌ 5 blocker(s) in 1 suite(s):
♻️ Settled after 2 auto re-run wave(s): #1 — 1 blocker + 1 broken suite(s); #2 — 1 blocker suite(s). |
|
/runall 🚀 RunAll queued — build 9228469. The verdict lands here when the run finishes. |
|
Ignite PR Checker verdict · RunAll build 9228469 · 147 suites ran, 0 reused ✅ No blockers — nothing in this run looks caused by this PR. 21 pre-existing/flaky tests filtered out. |
|
Ignite PR Checker verdict · RunAll build 9229249 · 16 suites ran, 131 reused ✅ No blockers — nothing in this run looks caused by this PR. 12 pre-existing/flaky tests filtered out. |
| for (int i = 1; i <= RMT_CNT; i++) | ||
| rmts.add(grid(i).localNode()); | ||
|
|
||
| MARSHAL_CNT.set(0); |
There was a problem hiding this comment.
In a clean run it cannot: MARSHAL_CNT is incremented only by CountingMarshaller, which is registered for direct type MAX_MESSAGE_ID + 1, and that type belongs only to MarshalOnceCheckMessage — created nowhere but in this test method. The reset matters because the counter is static, i.e. shared by all five nodes in the JVM and surviving across method boundaries: with @repeat (GridAbstractTest installs RepeatRule) or a repeated run of the class in the same fork the second iteration would enter with a non-zero value and the assert below would fail for the wrong reason.
|
|
||
| startGrids(2); | ||
|
|
||
| RETRY_MARSHAL_CNT.set(0); |
There was a problem hiding this comment.
Same as the other one: RETRY_MARSHAL_CNT is touched only by RetryCountingMarshaller, registered for direct type MAX_MESSAGE_ID + 2, which belongs only to RetryCheckMessage created in this method — so it is 0 in a clean run. The reset guards against the static counter carrying a value over from a repeated run in the same JVM.
| * objects travel as {@code QueryStartRequest} parameters and as {@code QueryBatchMessage} rows, and record the thread | ||
| * that deserializes them. | ||
| */ | ||
| public class CalciteMessageUnmarshalThreadIntegrationTest extends AbstractBasicIntegrationTest { |
There was a problem hiding this comment.
Can we put it in CalciteCommunicationMessageSerializationTest?
There was a problem hiding this comment.
They can't share a class — the bases are incompatible. CalciteCommunicationMessageSerializationTest extends AbstractMessageSerializationTest, a plain JUnit class that starts no nodes: it walks the registered direct types and round-trips each message through mock writer/reader, and it runs in the unit-level suite. The unmarshal-thread test extends AbstractBasicIntegrationTest (GridCommonAbstractTest), starts two servers plus a client and needs real SQL — CREATE TABLE with an OTHER column, 200 inserts and a query with a custom-object parameter — because it asserts on the threads in which the payload is actually deserialized. Putting it there would mean dragging the grid lifecycle into a unit-level serialization test and moving that class out of its suite.
| @@ -97,21 +110,32 @@ public class MessageProcessor extends AbstractProcessor { | |||
| static final String[] SKIP_MESSAGES = { | |||
There was a problem hiding this comment.
Maybe we should do smth. like registerExclusionMessages(Class<? extents Message> msgCls) instead
There was a problem hiding this comment.
A Class-typed API cannot be built here, in either direction. The codegen module depends only on ignite-commons, while Message lives in ignite-nio, so Class<? extends Message> does not even compile in this module. And the classes themselves are unreachable regardless of the bound: every excluded one lives in a module that depends on ignite-codegen (core, zookeeper, indexing, calcite), so codegen cannot depend back — that would be a Maven cycle. This is why the lists are FQN strings resolved through Elements.getTypeElement() with a null filter, so an entry missing from the current module's classpath is simply skipped. The mechanism is master's own; this PR adds a single entry to it.
|
Ignite PR Checker verdict · RunAll build 9232278 · 147 suites ran, 0 reused ✅ No blockers — nothing in this run looks caused by this PR. 28 pre-existing/flaky tests filtered out. |
| } | ||
|
|
||
| /** @return {@code true} if this message has been marshalled. */ | ||
| boolean marshalled() { |
There was a problem hiding this comment.
Is used only with asserts. Let's remove. We have the double0marshalling check mechanics
There was a problem hiding this comment.
These are the marshal side, and the double-unmarshal mechanics do not cover it: MessageUnmarshalOnceCheck tracks finish-unmarshal of a payload and is gated by IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK, i.e. it only runs under tests. The flag here guards three different invariants of the prepare-once scheme on the send path: marshal() asserts a wrap is never marshalled twice (marshalling is not idempotent — GridCacheEntryInfo rebases expireTime, for one), sendPrepared() asserts nothing unprepared is sent, and sendMarshalled() asserts nothing unmarshalled reaches the wire. That is what makes prepare-once safe for both fan-out (sendToMany) and retry (sendWithRetry), and it caught a real double-marshal while the retry loop was being reworked in this PR. MessageMarshalOnceTest covers two scenarios; the asserts cover every other path, including future ones. Assertions are off in production, so the cost is a boolean field on GridIoMessage.
| * <p>The rules key on whether the called method is {@code static}, not on its name — so any instance method added | ||
| * to these interfaces is covered automatically. | ||
| */ | ||
| public class MessageSerializationArchitectureTest { |
There was a problem hiding this comment.
Can we join it with MarshallerCacheFreeUnmarshallTest?
There was a problem hiding this comment.
They check different things: MarshallerCacheFreeUnmarshalTest verifies runtime behaviour of one path (a cache-free unmarshal leaves @Marshalled payload untouched and the cache-aware pass finishes it), while this class states three architectural rules over the whole org.apache.ignite class graph. Merging them would put a class-graph scan into a behavioural test and drag the ArchUnit import into it — the opposite of the concern in the other thread. Kept separate.
| import com.tngtech.archunit.base.DescribedPredicate; | ||
| import com.tngtech.archunit.core.domain.JavaClasses; | ||
| import com.tngtech.archunit.core.domain.JavaMethodCall; | ||
| import com.tngtech.archunit.core.domain.JavaModifier; |
There was a problem hiding this comment.
We bring new dependency/library only for tests, for 2 tests and only to check where we call own methods. I doubt it worths. @shishkovilja WDYT? If we aren'tsure how we call methods, probably it is design-related and should be reconsidered in general.
There was a problem hiding this comment.
The three rules pin the central invariant of this change: nobody outside the generated companions and the static facades may call MessageSerializer.writeTo/readFrom, MessageMarshaller.marshal/unmarshal or GridCacheMessageDeployer.deploy directly. That is not a stylistic wish — a direct call bypasses the dispatcher and, for the marshalling side, silently moves payload (de)serialization back onto the NIO thread and around the marshal-once contract. The rules have already earned their keep on this branch: every master merge that touched GridNioServer or the Calcite tests brought back a direct msgSer.writeTo(...) call, and the test caught it locally instead of in a suite run.
There is no design-level way to enforce it with the compiler: MessageSerializer is a public SPI, its implementations are generated into other packages, and factory.serializer(...) is called from GridNioServerWrapper (core) and MessageSerialization (nio), so the instance methods cannot be package-private or hidden.
Writing it without the library means bytecode analysis by hand (reflection cannot see call sites) — a home-grown ArchUnit of a couple hundred lines to maintain. The dependency is test-scoped and never ships; modules/core/pom.xml already carries com.google.testing.compile for exactly the same reason — a test-only library for one narrow check, and that one comes from master.
If "no new dependencies" is the priority, I'd rather file a follow-up to rewrite these three rules without the library than drop the check — losing it means the next such regression lands in master unnoticed. WDYT?
There was a problem hiding this comment.
@anton-vinogradov , I guggest to remove both tngtech test before the merging.
| this.last = last; | ||
|
|
||
| mRows = rows.stream().map(o -> o == null ? null : new GenericValueMessage(o)).collect(Collectors.toList()); | ||
| mRows = new ArrayList<>(rows.size()); |
There was a problem hiding this comment.
It is the same conversion as on master, just without the stream pipeline: master does mRows = rows.stream().map(o -> o == null ? null : new GenericValueMessage(o)).collect(toList()), this is the equivalent loop into a pre-sized ArrayList — no lambda/collector allocations per batch, same result. (The lazy variant I had here earlier is reverted, as agreed in the F.view thread.)
| */ | ||
| public List<Object> rows() { | ||
| return mRows.stream().map(GenericValueMessage::value).collect(Collectors.toList()); | ||
| List<Object> rows = new ArrayList<>(mRows.size()); |
There was a problem hiding this comment.
Same on the read side: master's rows() is mRows.stream().map(GenericValueMessage::value).collect(toList()), this is the same thing as a loop into a pre-sized list.
| * current one is answered with the key in {@code nearEvicted}, so the reader is dropped instead of keeping a stale | ||
| * near entry. | ||
| */ | ||
| public class GridNearTxRecreateEvictionTest extends GridCommonAbstractTest { |
There was a problem hiding this comment.
Can we put in other ticket?
| * An ordered message whose payload fails to unmarshal must be skipped with an error logged, not abandon the rest of | ||
| * the accumulated set: the tail would otherwise stay unprocessed until the next message arrives on the topic. | ||
| */ | ||
| public class GridIoManagerOrderedPoisonMessageTest extends GridCommonAbstractTest { |
There was a problem hiding this comment.
Why we keep creating test class for single test method? Let's put it somewhere aroud serialization/marshalling tests
There was a problem hiding this comment.
I'd keep it separate. The test needs two grids, a plugin registering its own direct type with a marshaller that fails on demand, and a listener thread blocked mid-delivery so that the next two messages pile up in one ordered set — that is the whole point of the scenario.
The neighbours around serialization/marshalling are unit-level: MessageProcessorTest (compile-testing), MarshallerCacheFreeUnmarshalTest and MessageSerializationArchitectureTest (class-graph rules), MessageUnmarshalOnceCheckTest — none of them start a node. The only grid-based one is MessageMarshalOnceTest, but its configuration installs a different plugin (two direct types with counting marshallers) plus an SPI that fails the first send; folding a third direct type and a blocking listener into it would merge two unrelated scenarios into one configuration. That is also the reasoning you applied yourself in the MessageSerializationArchitectureTest thread — different subjects, different classes.
|
|
||
| /** @return Number of messages pending in the receiver's ordered set of {@link #TOPIC}. */ | ||
| private static int pendingOrderedMessages(IgniteEx rcv) { | ||
| Map<Object, Map<UUID, Object>> setMap = GridTestUtils.getFieldValue(rcv.context().io(), "msgSetMap"); |
There was a problem hiding this comment.
We might cat to the implementation and add some getter with @TestOnly
There was a problem hiding this comment.
That was the shape this test had at first, and it was rejected in review: initMessageFactoryForTest() in IgniteKernal was removed on the grounds that a production class should not carry a method existing solely for one test, and the test was switched to reflection (U.invoke) instead — see the IgniteKernal thread. Adding a @testonly getter to GridIoManager for msgSetMap would walk that decision back, so I'd rather keep the reflection here: it is a test-only peek at internal state, contained in the test itself. Happy to switch if you and @shishkovilja agree on the opposite rule.
|
/runall 🚀 RunAll queued — build 9234302 · live progress & verdict: Ignite PR Checker. The verdict lands here when the run finishes. |
Possible compatibility issues. Please, check rolling upgrade casesThis PR modifies protected classes (with Order annotation). Affected files:
|
|
Ignite PR Checker verdict · RunAll build 9234928 · 147 suites ran, 0 reused ✅ No blockers — nothing in this run looks caused by this PR. 19 pre-existing/flaky tests filtered out. |
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.