anton-vinogradov commented on code in PR #13095:
URL: https://github.com/apache/ignite/pull/13095#discussion_r3649848406
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/GridDhtTxPrepareRequest.java:
##########
@@ -315,83 +305,18 @@ public boolean skipCompletedVersion() {
return txLbl;
}
- /**
- * {@inheritDoc}
- *
- * @param ctx
- */
- @Override public void prepareMarshal(GridCacheSharedContext<?, ?> ctx)
throws IgniteCheckedException {
- super.prepareMarshal(ctx);
-
+ /** {@inheritDoc} */
+ @Override public void deploy(GridCacheSharedContext<?, ?> ctx) throws
IgniteCheckedException {
if (owned != null && ownedKeys == null) {
- ownedKeys = owned.keySet();
-
- ownedVals = owned.values();
-
- for (IgniteTxKey key: ownedKeys) {
+ for (IgniteTxKey key : owned.keySet()) {
GridCacheContext<?, ?> cctx = ctx.cacheContext(key.cacheId());
- key.prepareMarshal(cctx);
-
if (addDepInfo)
- prepareObject(key, cctx);
+ deployObject(key, cctx);
Review Comment:
It actually is driven by the generated deployer:
GridDhtTxPrepareRequestDeployer deploys the type-inferable fields
(reads/writes/nearWrites via deployTx) and then delegates to this deploy(),
exactly like generated marshallers delegate the non-inferable part to
MarshallableMessage#marshal. The generator infers deployment from the field
type only (CacheObject, Collection of CacheObjects, Iterable<IgniteTxEntry>,
nested message). Here the target is the keys of Map<IgniteTxKey,
GridCacheVersion>, each resolved against its own cache via key.cacheId() (the
tx may span several caches, so there is no single msg.cacheId()) and guarded by
addDepInfo — none of that is derivable from the type, which is what the
DeployableMessage hook exists for. The body mirrors master's prepareMarshal
one-to-one.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/GridDhtLockResponse.java:
##########
@@ -124,24 +125,14 @@ public Collection<GridCacheEntryInfo> preloadEntries() {
}
/** {@inheritDoc} */
- @Override public void prepareMarshal(GridCacheSharedContext<?, ?> ctx)
throws IgniteCheckedException {
- super.prepareMarshal(ctx);
+ @Override public void deploy(GridCacheSharedContext<?, ?> ctx) throws
IgniteCheckedException {
+ if (preloadEntries != null) {
+ GridCacheContext<?, ?> cctx = ctx.cacheContext(cacheId);
- GridCacheContext<?, ?> cctx = ctx.cacheContext(cacheId);
-
- if (preloadEntries != null)
- marshalInfos(preloadEntries, cctx.shared(),
cctx.cacheObjectContext());
- }
-
- /** {@inheritDoc} */
- @Override public void finishUnmarshal(GridCacheSharedContext<?, ?> ctx,
ClassLoader ldr) throws IgniteCheckedException {
- super.finishUnmarshal(ctx, ldr);
-
- if (preloadEntries != null)
- unmarshalInfos(preloadEntries, ctx.cacheContext(cacheId), ldr);
+ deployInfos(preloadEntries, cctx);
Review Comment:
Same pattern as in GridDhtTxPrepareRequest: the generated
GridDhtLockResponseDeployer deploys the inferable field (vals) and then
delegates to this deploy(). preloadEntries is a Collection<GridCacheEntryInfo>,
and GridCacheEntryInfo is not a CacheObject — deploying it means unwrapping and
registering the key and value user objects of each info (deployInfos), which
cannot be inferred from the field type. This is the only message in the
codebase that deploys entry infos, so it stays behind the DeployableMessage
hook rather than a dedicated one-off generator strategy.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/GridDhtLockFuture.java:
##########
@@ -1033,9 +1033,6 @@ private void map(Iterable<GridDhtCacheEntry> entries) {
}
assert added.dhtLocal();
-
- if (added.ownerVersion() != null)
Review Comment:
This populated GridDhtLockRequest#owned, which was dead on the receiving
side: the only reader of owned() in master is
IgniteTxHandler#startNearRemoteTx, and it takes a GridDhtTxPrepareRequest
(populated in GridDhtTxPrepareFuture — that path is kept intact).
GridDhtLockRequest#owned was filled here, marshalled, unmarshalled — and never
read, so instead of porting a write-only field to codegen, the field and this
write were removed.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryBatchMessage.java:
##########
@@ -60,8 +66,7 @@ public QueryBatchMessage(UUID qryId, long fragmentId, long
exchangeId, int batch
this.exchangeId = exchangeId;
this.batchId = batchId;
this.last = last;
-
- mRows = rows.stream().map(o -> o == null ? null : new
GenericValueMessage(o)).collect(Collectors.toList());
+ this.rows = rows;
Review Comment:
mRows must be a materialized list. On the send path it is iterated twice by
two different actors: the generated QueryBatchMessageMarshaller.marshal walks
mRows and marshals each wrapper (fills its serialized bytes), then
QueryBatchMessageSerializer.writeTo iterates it again via
writer.writeCollection. A lazy view (F.view/F.viewReadOnly) produces new
GenericValueMessage instances on every iteration, so the second pass would
write wrappers whose serialized == null — empty payload on the wire.
F.transform is eager and would work, but it returns Collection while the field
and the consumers are typed List, so it is not a drop-in either; keeping the
explicit loop.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryBatchMessage.java:
##########
@@ -99,6 +104,29 @@ public boolean last() {
* @return Rows.
*/
public List<Object> rows() {
- return
mRows.stream().map(GenericValueMessage::value).collect(Collectors.toList());
+ return rows;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void marshal(Marshaller marsh) {
Review Comment:
It is marshallable in fact: every row ends up serialized by the Marshaller —
GenericValueMessage.marshal does serialized = U.marshal(marsh, val), driven by
the generated QueryBatchMessageMarshaller which first calls
msg.marshal(marshaller) (the rows -> wrappers conversion) and then marshals
each element of mRows. Rows are arbitrary objects the direct protocol cannot
write; the marshal/unmarshal hooks are the only point in the cascade where the
rows<->wrappers conversion can happen, so the interface cannot be dropped here.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryBatchMessage.java:
##########
@@ -99,6 +104,29 @@ public boolean last() {
* @return Rows.
*/
public List<Object> rows() {
- return
mRows.stream().map(GenericValueMessage::value).collect(Collectors.toList());
+ return rows;
Review Comment:
Lazy init would not buy anything here: rows() is consumed immediately after
unmarshal on the same query-executor stripe —
MessageServiceImpl.onMessageInternal unmarshals and right away invokes the
listener, which calls inbox.onBatchReceived(..., msg.rows()). The conversion
would just happen a few frames later on the same thread, while the message
would additionally keep mRows alive. F.transform returns Collection, while rows
is a List consumed as List<Row> by Inbox.onBatchReceived, so the explicit loop
stays.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/continuous/CacheContinuousQueryEntry.java:
##########
@@ -58,25 +58,25 @@ public class CacheContinuousQueryEntry implements
GridCacheDeployable, Marshalla
@GridToStringInclude
KeyCacheObject key;
- /** */
+ /** {@code null} for a filtered entry. */
@Order(2)
- byte[] keyBytes;
+ KeyCacheObject keyWire;
Review Comment:
This is the same local/wire field pair master already has: key + @Order
byte[] keyBytes, copied in prepareMarshal(Marshaller) under !isFiltered();
before the serializer extraction the hand-written writeTo() did
writer.writeKeyCacheObject(isFiltered() ? null : key). The wire contract is
that a filtered entry carries no key/values, while the local instance must keep
them: the entry gets marked filtered after it was created with data (filter(),
skipUpdateEvent, ContinuousQueryAsyncClosure on a failed DHT future), and they
are still read afterwards — e.g. onEntryUpdate records CacheQueryReadEvent from
evt.getKey()/getValue()/getOldValue() on that path; data stripping is only done
on explicit copyWithDataReset() copies. The generated serializer writes @Order
fields unconditionally (there is no per-field converter facility), so the
isFiltered() ? null : key conditional lives in the marshal() hook and needs a
separate wire field — same companion-field concept as @Marshalled, just wit
h a condition. The only change against master is the wire form: the typed
KeyCacheObject/CacheObject is written directly instead of an extra marshaller
round-trip to byte[].
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/continuous/CacheContinuousQueryEntry.java:
##########
@@ -58,25 +58,25 @@ public class CacheContinuousQueryEntry implements
GridCacheDeployable, Marshalla
@GridToStringInclude
KeyCacheObject key;
- /** */
+ /** {@code null} for a filtered entry. */
@Order(2)
- byte[] keyBytes;
+ KeyCacheObject keyWire;
/** New value. */
@GridToStringInclude
CacheObject newVal;
- /** */
+ /** {@code null} for a filtered entry. */
@Order(3)
- byte[] newValBytes;
+ CacheObject newValWire;
Review Comment:
Same as the thread below (line 63): the master's local/wire pair (key +
keyBytes), only with a typed wire form.
--
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]
