IGNITE-28528 Revise marshalling of the node filter in StartRequestData - #13440
IGNITE-28528 Revise marshalling of the node filter in StartRequestData#13440anton-vinogradov wants to merge 25 commits into
Conversation
The sender of the classes is already inside the descriptor: the class loader id carries it, and the loader asserts as much. So getGlobalDeployment no longer asks for it, and the dead nodeFilter parameter is gone with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GridEventStorageRequest kept the deployment as four flat fields, so it had nothing to pass as a descriptor. It now carries GridDeploymentInfoBean, like the messages that already do, and resolves through the descriptor. 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>
…lers A message that carries classes deployed on another node now says so with DeploymentAware, the same way CacheIdAware states the need for a cache context. The generated marshaller asks the deployment manager for the class loader, so a receiver no longer repeats that resolution by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keeping the serialized copy is now a property of the field, @Marshalled keepBytes, instead of a property of the deferred-unmarshal marker: the marker covers GridCacheMessage, so tying it there dropped the memory saving for every cache message. Restores the diagnostics of two error paths, adds a codegen test for the resolved class loader and for keepBytes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Marking StartRequestData deployment-aware put the deployment resolution into its generated unmarshal, and that unmarshal runs from the discovery socket reader, where obtaining a deployment may block on a network request. The message keeps its deployment as a plain field instead and asks the deployment manager for the loader where blocking is allowed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The socket-reading threads of both discovery SPIs unmarshalled every message they read. A continuous-query start request cannot be read there: obtaining the deployment of its node filter may block on a network request, and a missing user class has to be tolerated so the acknowledgement is still sent. Both readers now honour DeferredUnmarshalMessage, as the communication manager already did, and the continuous processor reads the request where that work is allowed. StartRequestData is left as plain data: its marshal/unmarshal methods are gone, the fields are marshalled by the generated marshaller, and preparing the handler moved to the processor that builds and consumes the message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generated marshaller drops the serialized copy of a field once the object is restored, which a message that travels the discovery ring cannot afford: every node would pass on a copy of its own making. For the handler that is not even equivalent - p2pUnmarshal unpacks its user objects and nothing packs them back, so they would go on the wire outside peer class loading. The request stays plain data then: the fields keep their serialized form, and the continuous processor restores the objects, asking the deployment manager for the class loader of the node filter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Whether the serialized copy of a field may be dropped once the object is restored is a property of the message, not of the field: a message read and then sent on must reach every node in the form it left the sender. ForwardedMessage states that, so the start request goes back on the generated marshaller, and its handler keeps the bytes it arrived with - p2pUnmarshal unpacks the user objects inside it and nothing packs them back. Reading it is deferred, as before: both discovery readers honour the marker, and the continuous processor reads the message where obtaining a deployment is allowed to block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The handler survives being read and sent on: its writeExternal writes the deployable wrapper whenever there is one, so what p2pUnmarshal restores never reaches the wire. The node filter is simply marshalled again, as it is on every other discovery message with a marshalled field. Keeping the serialized copy was guarding against nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…escriptor # Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/processors/datastreamer/DataStreamerRequest.java
Fixes the import order the strict checkstyle profile rejects, and drops what earlier iterations left behind: an unused constant, an unused overload, and a javadoc line describing a design that is no longer there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ator Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The internal code uses the Bean suffix nowhere else: what is left is Spring, public API and JMX. This class is a Message carrying a deployment descriptor inside other messages, like BinaryMetadataVersionInfo or NodeEncryptionKeys, and its name now says so. It cannot simply drop the suffix, since it implements the GridDeploymentInfo interface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ler passed The generated resolution ran only when the caller passed no class loader, but the convenience overloads pass the local one, so a caller using them would read peer-deployed classes without peer class loading. A message that carries its own deployment knows the loader better than its caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/runall 🎨 Checkstyle autofix: fixed 2 of 15 violation(s) in 2 file(s) — commit 7454525. 13 remain (javadoc/naming/wrapping need a human) — the Checkstyle suite may still fail. |
|
Ignite PR Checker verdict · RunAll build 9266790 · 147 suites ran, 0 reused
Everything below is what it did manage to say. 👀 6 test(s) started failing on this code — not proven blockers yet: too few runs of this revision to tell a break from a flake, so a re-run of the suite decides it.
⏳ Auto re-run #2 in progress — 2 blocker suite(s) re-queued (attempt 2/2), ≈ settled by 09:13 MSK. This comment updates when they settle. |
A discovery custom message is not carried as bytes: TcpDiscoveryCustomEventMessage holds it as a nested message field, so the generated marshaller of the envelope reads the whole tree on the socket-reading thread. There a missing user class is swallowed with a warning, and the routine start never reports it back to the node that started it. The node filter and the handler therefore stay serialized on the message, and the continuous processor restores them where the failure can be reported. The deployment resolution is still a single call, which is what the ticket asks for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ignite-deployment-descriptor
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Ignite PR Checker verdict · RunAll build 9267428 · 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. 25 pre-existing/flaky tests filtered out. Re-run once the above is sorted out. ♻️ Settled after 1 auto re-run wave(s): #1 — 4 watch suite(s). |
|
Ignite PR Checker verdict · RunAll build 9268108 · 147 suites ran, 0 reused ✅ No blockers — nothing in this run looks caused by this PR. 18 pre-existing/flaky tests filtered out. ♻️ Settled after 1 auto re-run wave(s): #1 — 3 watch suite(s). |
| rsrcName, | ||
| clsName, | ||
| depInfo.userVersion(), | ||
| depInfo.classLoaderId().globalId(), |
There was a problem hiding this comment.
It's confusing that depInfo.classLoaderId().globalId() is used as sndNodeId. Why is that?
There was a problem hiding this comment.
The two are the same node by construction, so this was not a shortcut - but you are right that nothing at the call site said so.
A class loader id carries the id of the node that created it, and GridDeploymentClassLoader asserts it in both places where such a loader is built:
assert nodeId.equals(clsLdrId.globalId()); // :184, constructor
assert nodeId.equals(ldrId.globalId()); // :324, register(nodeId, ldrId), for a participantSo a message describing a deployment cannot name a sender that disagrees with the loader it describes: the old code, which passed the sender separately, blew up on that assert if the two ever diverged.
I have made it readable instead of implied: GridDeploymentInfo#nodeId() now returns it, with the invariant written in its javadoc, and the call site reads depInfo.nodeId().
There was a problem hiding this comment.
I was wrong here, and your question is what led me to it - thanks.
The two nodes are not the same in general. GridCacheDeploymentManager states it directly:
assert sndId.equals(ldrId.globalId()) || participants != null;and GridDeploymentManager#deploy hands out the deployment of the original owner when the class arrived by peer loading - the comment there calls it a nested execution:
// Check for nested execution. In that case, if task
// is available locally by name, then we should ignore class loader ID.
dep = checkDeployment(ldrStore.getDeployment(ldr.classLoaderId()), "perLoader");So a node that received classes from another one passes them further as a participant of the same deployment, while being the sender itself.
And the receiving side needs the sender, not the owner: GridDeploymentPerVersionStore asks it for the classes, records it with addParticipant, and refuses the deployment when discovery().node(senderNodeId) == null. Deriving the node from the loader id would break exactly the case where the owner has left and the sender still holds the classes.
Fixed: the resolution takes the node as a parameter again. Since generated code has no way to know the sender, the DeploymentAware interface is gone with it, and the callers - which do know the sender - pass it in.
|
|
||
| if (dep == null) | ||
| throw new IgniteDeploymentCheckedException("Failed to obtain deployment for class: " + clsName); | ||
| GridDeployment dep = ctx.deploy().globalDeployment(depInfo, clsName); |
There was a problem hiding this comment.
Was the nodeId method parameter lost during refactoring, or is it no longer required and should be deleted?
There was a problem hiding this comment.
No longer required - and it turned out to be true well past this method.
The deployment is now resolved from the descriptor the message carries, so the node id has nothing to do here. Following it upwards, it had nothing to do anywhere else either: it went through GridContinuousHandler#p2pUnmarshal, its three implementations and the private helper in CacheContinuousQueryHandler, and every one of them only fed it to an assert nodeId != null.
Deleted along the whole chain, so the signature is p2pUnmarshal(GridKernalContext) now.
There was a problem hiding this comment.
Correction to my previous reply: the parameter is back.
The deployment resolution needs the sending node after all - it is not derivable from the loader id, see the other thread. So p2pUnmarshal(UUID nodeId, GridKernalContext ctx) keeps its signature, and the node is passed down to the resolution instead of only feeding an assert.
|
Generated code for Why is |
The deployment resolution no longer takes the sender node id, so the parameter had nothing left to do: it travelled through the continuous handler interface, three implementations and a private helper, used by an assert alone. Where the id is still needed, GridDeploymentInfo#nodeId now says where it comes from, instead of reaching into the loader id at the call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| return getGlobalDeployment(depInfo.deployMode(), | ||
| rsrcName, | ||
| clsName, | ||
| depInfo.userVersion(), |
There was a problem hiding this comment.
Is depInfo guaranteed to be non-null here? In some places it seems that it can be null, for example in org.apache.ignite.internal.managers.communication.GridIoUserMessage#deploymentInfo.
There was a problem hiding this comment.
It cannot be null today, but only by an invariant that is nowhere stated - you are right not to trust it.
On the sending side both fields come from the same deployment, and the send fails outright when there is none:
dep = ctx.deploy().deploy(cls0, U.detectClassLoader(cls0));
if (dep == null)
throw new IgniteDeploymentCheckedException(...);
depClsName = cls0.getName();So deploymentClassName() != null implies deploymentInfo() != null, and the guard on the receiving side happens to check the first while the code dereferences the second.
Rather than document that, I made it structural: the resolution now accepts a null deployment and returns null for it, so nothing to resolve gives nothing back, and the caller reports it with the message it already has. The throwing overload turns the same case into its usual IgniteDeploymentCheckedException.
The sender and the owner of a class loader are not the same node in general: a node that got classes by peer loading passes them on as a participant, which GridCacheDeploymentManager states outright, and GridDeploymentManager#deploy hands out the deployment of the original owner for a nested execution. The receiving side needs the sender, not the owner: that is who is asked for the classes, who is recorded as a participant, and whose departure makes the deployment unusable. Deriving it from the loader id was therefore wrong, and no generated code can supply it, so DeploymentAware is gone and the resolution takes the node as a parameter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A message may carry none - GridIoUserMessage does when peer class loading is off - and the caller then relied on a second field being null in step. Resolving nothing now yields nothing, so the caller reports it the way it already does instead of depending on that. 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:
|
Good catch, and it was a symptom of the same thing your inline question uncovered - that generated code cannot pick the class loader for a message. Overwriting the parameter was deliberate but wrong. It came from default void unmarshal(M msg, GridKernalContext kctx) {
unmarshal(msg, kctx, null, U.resolveClassLoader(kctx.config()));
}so a caller using them would silently read peer-deployed classes with the local loader. Ignoring the argument fixed that symptom and hid a worse problem: the resolution needs the sending node, which the generated code has no way to know, and the sender is not always the owner of the class loader. So the whole thing is gone now. What is left is what the ticket asked for: the deployment travels as one descriptor instead of four flat fields, and the eight places that used to assemble it by hand now make a single call. The diff went from 35 files to 25, and from +577/-497 to +248/-475. Since the change is materially different from what the last RunAll saw, a fresh run is needed. |
|
Ignite PR Checker verdict · RunAll build 9269815 · 147 suites ran, 0 reused
Everything below is what it did manage to say. 🔍 1 suite(s) ran fewer tests than on master (tests that never ran can't fail):
🔎 No blockers found — but the run above can't prove the PR is clean. 20 pre-existing/flaky tests filtered out. Re-run once the above is sorted out. ♻️ Settled after 2 auto re-run wave(s): #1 — 1 broken suite(s); #2 — 1 broken suite(s). |
2 participants
The node filter in
StartRequestDatawas marshalled by hand, and the receiverhad to build the deployment class loader itself before reading the filter.
Why the code looked like that
A message that carries peer-deployed classes said nothing about it. It held the
deployment as four or five flat fields, so every receiver rebuilt the deployment
from those fields and passed the resulting loader into the unmarshal call. Eight
places did exactly that.
StartRequestDataalso repeated a "peer class loadingis off" branch, although
GridDeploymentManageralready covers that case: withpeer class loading off it looks the deployment up locally.
Change
DeploymentAwarelets a message state that its classes come with a deployment,the same way
CacheIdAwarestates the need for a cache object context.GridDeploymentManager.classLoader(DeploymentAware)is now the single placethat turns a carried deployment into a class loader.
receivers no longer resolve it by hand.
GridDeploymentInfoBeanas a whole instead of the flat fields.This changes the wire format.
DeploymentAwareis independent ofDeferredUnmarshalMessage, which says whostarts the read: a receiver that reports a missing deployment back to the sender
has to start the read itself, inside its own error handling.
Reading a discovery message off the reading thread
The socket-reading threads of both discovery SPIs unmarshalled every message
they read. A continuous-query start request cannot be read there: obtaining the
deployment of its node filter may block on a network request, and a missing user
class has to be tolerated so the acknowledgement is still sent. Both readers now
honour
DeferredUnmarshalMessage, asGridIoManageralready did, and thecontinuous processor reads the request where that work is allowed.
StartRequestDatais then left as plain data. Itsmarshal/unmarshalmethodsare gone: the fields are marshalled by the generated marshaller, and preparing
the handler moved to the processor that builds and consumes the message.
Verified
MessageProcessorTest(a new case covers the resolved loader andkeepBytes),GridEventStorageCheckAllEventsSelfTest,GridEventStorageManagerSelfTest,GridEventConsumeSelfTest,GridMessageListenSelfTest,GridP2PContinuousDeploymentSelfTest,P2PClassLoadingFailureHandlingTest,IgniteNoCustomEventsOnNodeStart,DataStreamerImplSelfTest,GridP2PContinuousDeploymentClientDisconnectTest, and the continuous-querysuites
CacheContinuousQueryOperationP2PTest,CacheContinuousQueryOperationFromCallbackTest,CacheContinuousQueryFailoverAtomicSelfTest,GridCacheContinuousQueryNodesFilteringTest,IgniteCacheContinuousQueryClientTest,IgniteCacheContinuousQueryReconnectTest,ContinuousQueryBuffersCleanupTest.🤖 Generated with Claude Code