anton-vinogradov commented on code in PR #13095: URL: https://github.com/apache/ignite/pull/13095#discussion_r3649847997
########## modules/core/src/test/resources/codegen/TestMarshalledCollectionMessage.java: ########## @@ -0,0 +1,34 @@ +/* + * 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 java.util.Set; +import org.apache.ignite.internal.processors.cache.version.GridCacheVersion; +import org.apache.ignite.plugin.extensions.communication.Message; + +public class TestMarshalledCollectionMessage implements Message { Review Comment: The generator does not discriminate collection interfaces: any Collection subtype takes the same code path (erasure check + (Collection<? extends T>) cast), and the serializer-side Set-vs-List distinction (the set flag of the collection descriptor) is covered by TestCollectionsMessage (23 List + 3 Set fields). The @Marshalled flavours are keyed by the wire-field shape and all four had dedicated test messages. The one generator branch without a golden was the array-backed map reconstruction (as in TxLocksResponse.nearTxKeyLocks) — added TestMarshalledArrayMapMessage covering it, both the regular and the final-map form. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheObjectNotResolvedException.java: ########## @@ -0,0 +1,32 @@ +/* + * 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.processors.cache; + +/** + * Thrown by {@link KeyCacheObjectImpl#hashCode()} when the object has not been deserialized yet, + * which happens when the owning cache has been removed before {@code unmarshal} could complete. + */ +public class CacheObjectNotResolvedException extends RuntimeException { Review Comment: It has to be unchecked — it is thrown from KeyCacheObjectImpl#hashCode(), which cannot declare checked exceptions. On master this situation was `assert val != null`: with assertions enabled the AssertionError escapes into onMessage0's catch-all and trips the failure processor (node stop); without them IgniteUtils.hashCode(null) silently returns 0. The dedicated runtime exception is caught in GridCacheIoManager.unmarshall next to BinaryObjectException and converted to onClassError, so the node answers via processFailedMessage instead of failing. It extends RuntimeException directly because it uses the stackless constructor, which IgniteException does not expose; the throw site is unique and self-describing (can switch the base if you prefer it over the stackless property). Negative-path coverage added: GridNearTxPrepareDestroyedCacheTest drives exactly this exception through that catch. ########## modules/core/src/test/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryResponseUnmarshalTest.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.processors.cache.query; + +import java.util.List; +import java.util.Map; +import org.apache.ignite.internal.GridKernalContext; +import org.apache.ignite.internal.managers.communication.MessageMarshalling; +import org.apache.ignite.internal.processors.cache.CacheObjectNotResolvedException; +import org.apache.ignite.internal.processors.cache.GridCacheContext; +import org.apache.ignite.internal.processors.cache.KeyCacheObject; +import org.apache.ignite.internal.processors.cache.KeyCacheObjectImpl; +import org.apache.ignite.internal.util.typedef.T2; +import org.apache.ignite.testframework.GridTestUtils; +import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest; +import org.junit.Test; + +/** + * Verifies that a query result row key arrives resolved from {@code GridCacheQueryResponse} unmarshalling: a + * {@code KeyCacheObject} travels bytes-only and forbids lazy resolution (see {@code @Marshalled}). + */ +public class GridCacheQueryResponseUnmarshalTest extends GridCommonAbstractTest { + /** An unresolved key (only its bytes present) must fail fast rather than resolve lazily. */ + @Test + public void testUnresolvedKeyThrows() { + KeyCacheObject unresolved = new KeyCacheObjectImpl(null, new byte[] {1, 2, 3}, -1); + + GridTestUtils.assertThrows(log, unresolved::hashCode, CacheObjectNotResolvedException.class, null); + } + + /** {@inheritDoc} */ + @Override protected void afterTest() throws Exception { + super.afterTest(); + + stopAllGrids(); + } + + /** */ + @Test + public void testRowKeyResolved() throws Exception { Review Comment: Added GridNearTxPrepareDestroyedCacheTest: it blocks a GridNearTxPrepareRequest carrying a TRANSFORM entry, destroys the cache (no recreate), releases the message and asserts the receiver answers with a GridNearTxPrepareResponse whose cause is CacheObjectNotResolvedException, while its failure handler is not triggered and the cluster stays alive (cache is recreated and used afterwards). Without this exception in the catch the test fails exactly the way you were worried about: the error escapes to the FailureProcessor and kills the node. The generic mechanism was also already exercised end-to-end by IgniteCacheP2pUnmarshallingErrorTestSuite via the neighboring catch. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/IgniteTxHandler.java: ########## @@ -430,12 +432,21 @@ public IgniteInternalFuture<GridNearTxPrepareResponse> prepareNearTxLocal( IgniteTxEntry firstEntry; try { - IgniteTxEntry firstWrite = unmarshal(req.writes()); - IgniteTxEntry firstRead = unmarshal(req.reads()); + IgniteTxEntry firstWrite = initialize(req.writes(), req.topologyVersion()); + IgniteTxEntry firstRead = initialize(req.reads(), req.topologyVersion()); firstEntry = firstWrite != null ? firstWrite : firstRead; } catch (IgniteCheckedException e) { + try { + req.onClassError(e); Review Comment: Sure, short version: with codegen serialization the entry-to-cache-context binding moved from GridCacheIoManager.unmarshall into this handler, so errors like "cache recreated" are now thrown here — past the IoManager catch that used to send the failure response. This catch restores that behavior: tell the near node the prepare failed instead of leaving it waiting forever. Without it IgniteCacheRecreateTest hangs (checked 3/3 runs both ways). ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/IgniteTxHandler.java: ########## @@ -1215,7 +1229,29 @@ private void processDhtTxPrepareRequest(final UUID nodeId, final GridDhtTxPrepar if (nearTx != null) res.nearEvicted(nearTx.evicted()); - List<IgniteTxKey> writesCacheMissed = req.nearWritesCacheMissed(); + List<IgniteTxKey> writesCacheMissed = new ArrayList<>(); + + Collection<IgniteTxEntry> writes = req.nearWrites(); + + for (Iterator<IgniteTxEntry> it = writes.iterator(); it.hasNext();) { + IgniteTxEntry e = it.next(); + + GridCacheContext<?, ?> cacheCtx = ctx.cacheContext(e.cacheId()); + + // A missing cache or one recreated after the request was sent: report the key back as evicted, + // so the near node drops its stale entry. + if (cacheCtx == null Review Comment: The cacheCtx == null part is the old finishUnmarshal logic moved here verbatim (context binding now happens in the handler). The extra topologyVersion().before(startTopologyVersion()) condition is intentional: destroy+recreate of a cache keeps its cacheId (derived from the name), so the null check alone does not catch a prepare request addressed to the previous cache incarnation — such a stale near write silently passed and the reader kept a stale entry instead of being evicted. Now these keys are reported back in nearEvicted; GridNearTxRecreateEvictionTest covers exactly this scenario. ########## modules/core/src/test/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxRecreateEvictionTest.java: ########## @@ -0,0 +1,136 @@ +/* + * 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.processors.cache.distributed.near; + +import java.util.ArrayList; +import java.util.List; +import org.apache.ignite.IgniteCache; +import org.apache.ignite.configuration.CacheConfiguration; +import org.apache.ignite.configuration.IgniteConfiguration; +import org.apache.ignite.configuration.NearCacheConfiguration; +import org.apache.ignite.internal.IgniteEx; +import org.apache.ignite.internal.IgniteInternalFuture; +import org.apache.ignite.internal.TestRecordingCommunicationSpi; +import org.apache.ignite.internal.processors.cache.distributed.dht.GridDhtTxPrepareRequest; +import org.apache.ignite.internal.processors.cache.distributed.dht.GridDhtTxPrepareResponse; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.testframework.GridTestUtils; +import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest; +import org.apache.ignite.transactions.Transaction; +import org.junit.Test; + +import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL; +import static org.apache.ignite.cache.CacheWriteSynchronizationMode.FULL_SYNC; +import static org.apache.ignite.transactions.TransactionConcurrency.OPTIMISTIC; +import static org.apache.ignite.transactions.TransactionIsolation.READ_COMMITTED; + +/** + * Checks that a near-write {@link GridDhtTxPrepareRequest} targeting a cache incarnation older than the receiver's + * 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 { Review Comment: Yes — a latent gap spotted during self-review while porting the finishUnmarshal logic into the handler (not a regression of this PR, master has it too). Master only dropped near writes when cacheCtx == null, but a cache recreated under the same name keeps its cacheId, so a prepare request addressed to the previous incarnation passed the check and the near reader silently kept a stale entry instead of being evicted. The test pins the fixed behavior: such a stale near write must come back in nearEvicted. -- 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]
