anton-vinogradov commented on code in PR #13095: URL: https://github.com/apache/ignite/pull/13095#discussion_r3649849133
########## modules/core/src/test/resources/codegen/NioFieldOnNonMessageMessage.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal; + +import org.apache.ignite.plugin.extensions.communication.Message; + +public class NioFieldOnNonMessageMessage implements Message { Review Comment: The name mirrors the asserted diagnostic: the test checks the "@NioField has no effect on non-Message field" error, i.e. @NioField placed on a field whose type is not a Message. "NioFieldOnFieldMessage" would drop exactly that discriminator — every @NioField sits on a field. It also lines up with the sibling negative resources (CustomEnumMapperOnPrimitiveFieldMessage, CustomEnumMapperOnArrayFieldMessage, NioFieldNeedsCtxMessage). If the double "MessageMessage" reads awkwardly, NioFieldOnNonMessageFieldMessage would be the convention-matching alternative. ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/CalciteErrorMessage.java: ########## @@ -18,11 +18,12 @@ package org.apache.ignite.internal.processors.query.calcite.message; import java.util.UUID; +import org.apache.ignite.internal.DeferredUnmarshalMessage; import org.apache.ignite.internal.Order; import org.apache.ignite.internal.managers.communication.ErrorMessage; /** */ -public class CalciteErrorMessage extends ErrorMessage { +public class CalciteErrorMessage extends ErrorMessage implements DeferredUnmarshalMessage { Review Comment: All Calcite messages implement DeferredUnmarshalMessage deliberately — the marker does two jobs: 1. Keeps payload deserialization off NIO threads. Calcite messages travel on TOPIC_QUERY with GridIoPolicy.CALLER_THREAD, which maps to SameThreadExecutor, so the generic inbound pass (GridIoManager.processRegularMessage0 -> unmarshalPayload) would run right on the NIO reader. For messages with real payload — QueryStartRequest parameters, QueryBatchMessage rows, the Throwable inside CalciteErrorMessage/QueryStartResponse — that would deserialize user objects on a NIO thread, the very thing the pre-patch CalciteContextMarshallableMessage existed to prevent. CalciteMessageUnmarshalThreadIntegrationTest guards exactly this. 2. Serves as the ownership filter on the shared topic. TOPIC_QUERY is also listened to by IgniteH2Indexing and RunningQueryManager; MessageServiceImpl.onMessage accepts a message only if it is instanceof DeferredUnmarshalMessage (previously: CalciteMessageFactory.isCalciteMessage). Drop the marker from any Calcite message and the listener silently ignores it. For the payload-free ones (e.g. QueryCloseMessage) the deferred unmarshal is a no-op — no marshaller is generated for them, so the dispatch resolves nothing and returns. The marker costs nothing there while keeping the invariant the listener filter relies on. ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryCloseMessage.java: ########## @@ -18,13 +18,13 @@ package org.apache.ignite.internal.processors.query.calcite.message; import java.util.UUID; +import org.apache.ignite.internal.DeferredUnmarshalMessage; import org.apache.ignite.internal.Order; -import org.apache.ignite.plugin.extensions.communication.Message; /** * */ -public class QueryCloseMessage implements Message { +public class QueryCloseMessage implements DeferredUnmarshalMessage { Review Comment: Not unnecessary: besides deferring payload deserialization off the NIO thread (CALLER_THREAD policy = SameThreadExecutor), the marker is what MessageServiceImpl.onMessage uses to pick Calcite messages out of the shared TOPIC_QUERY (H2 and RunningQueryManager listen there too). Without it the message is silently dropped. Full details in the CalciteErrorMessage thread. ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryBatchMessage.java: ########## @@ -17,13 +17,16 @@ package org.apache.ignite.internal.processors.query.calcite.message; +import java.util.ArrayList; import java.util.List; import java.util.UUID; -import java.util.stream.Collectors; +import org.apache.ignite.internal.DeferredUnmarshalMessage; +import org.apache.ignite.internal.MarshallableMessage; import org.apache.ignite.internal.Order; +import org.apache.ignite.marshaller.Marshaller; /** */ -public class QueryBatchMessage implements ExecutionContextAware { +public class QueryBatchMessage implements MarshallableMessage, DeferredUnmarshalMessage, ExecutionContextAware { Review Comment: Covered in the CalciteErrorMessage thread: the marker both defers payload deserialization off the NIO thread and is the receive filter on the shared TOPIC_QUERY — dropping it means the listener silently ignores the message. ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryStartResponse.java: ########## @@ -18,15 +18,15 @@ package org.apache.ignite.internal.processors.query.calcite.message; import java.util.UUID; +import org.apache.ignite.internal.DeferredUnmarshalMessage; import org.apache.ignite.internal.Order; import org.apache.ignite.internal.managers.communication.ErrorMessage; -import org.apache.ignite.plugin.extensions.communication.Message; import org.jetbrains.annotations.Nullable; /** * */ -public class QueryStartResponse implements Message { +public class QueryStartResponse implements DeferredUnmarshalMessage { Review Comment: Covered in the CalciteErrorMessage thread: the marker both defers payload deserialization off the NIO thread (this message carries an ErrorMessage with a marshalled Throwable) and is the receive filter on the shared TOPIC_QUERY. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheEntryInfo.java: ########## @@ -201,26 +191,8 @@ public int marshalledSize(CacheObjectContext ctx) throws IgniteCheckedException return SIZE_OVERHEAD + size; } - /** - * @param ctx Cache context. - * @throws IgniteCheckedException In case of error. - */ - public void marshal(GridCacheContext ctx) throws IgniteCheckedException { - marshal(ctx.cacheObjectContext()); - } - - /** - * @param ctx Cache context. - * @throws IgniteCheckedException In case of error. - */ - public void marshal(CacheObjectContext ctx) throws IgniteCheckedException { - assert key != null; - - key.prepareMarshal(ctx); - - if (val != null) - val.prepareMarshal(ctx); - + /** {@inheritDoc} */ + @Override public void marshal(Marshaller marsh) throws IgniteCheckedException { Review Comment: Right — the hooks here never touch the Marshaller: they only rebase expireTime between the absolute and remaining-TTL form so the value survives node clock differences. Filing a subticket to revise this (same direction as IGNITE-28914). -- 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]
