animovscw commented on code in PR #12841: URL: https://github.com/apache/ignite/pull/12841#discussion_r3032772819
########## modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/ZookeeperDiscoveryMultiJvmNodeRunnerReproducerTest.java: ########## @@ -0,0 +1,193 @@ +/* + * 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.spi.discovery.zk; + +import org.apache.curator.test.TestingCluster; +import org.apache.ignite.Ignite; +import org.apache.ignite.configuration.IgniteConfiguration; +import org.apache.ignite.internal.processors.resource.GridSpringResourceContext; +import org.apache.ignite.spi.discovery.DiscoverySpi; +import org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi; +import org.apache.ignite.spi.discovery.tcp.ipfinder.TcpDiscoveryIpFinder; +import org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder; +import org.apache.ignite.testframework.GridTestUtils; +import org.apache.ignite.testframework.config.GridTestProperties; +import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest; +import org.apache.ignite.testframework.junits.multijvm.IgniteProcessProxy; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Reproducer for IGNITE-27016. + * + * <p>This test starts the second node in a separate JVM and validates that + * discovery SPI preprocessed to {@link ZookeeperDiscoverySpi} is preserved + * through remote-node configuration serialization.</p> + * + * <p>Expected behavior: + * with fix - node joins successfully; + * without fix - remote startup fails (e.g. "Remote node has not joined").</p> + */ +public class ZookeeperDiscoveryMultiJvmNodeRunnerReproducerTest extends GridCommonAbstractTest { + /** */ + private static final TcpDiscoveryIpFinder IP_FINDER = new TcpDiscoveryVmIpFinder(true); + + /** */ + private static TestingCluster testingCluster; + + /** */ + private static String oldZkForceSync; + + /** */ + private static String oldCfgPreprocessor; + + /** */ + @BeforeClass + public static void beforeClass() throws Exception { + oldZkForceSync = System.getProperty("zookeeper.forceSync"); + oldCfgPreprocessor = System.getProperty(GridTestProperties.IGNITE_CFG_PREPROCESSOR_CLS); + + System.setProperty("zookeeper.forceSync", "false"); + + testingCluster = ZookeeperDiscoverySpiTestUtil.createTestingCluster(3); + testingCluster.start(); + + System.setProperty( + GridTestProperties.IGNITE_CFG_PREPROCESSOR_CLS, + ZookeeperDiscoveryMultiJvmNodeRunnerReproducerTest.class.getName() + ); + } + + /** */ + @AfterClass + public static void afterClass() throws Exception { + if (testingCluster != null) + testingCluster.close(); + + testingCluster = null; + + if (oldZkForceSync != null) + System.setProperty("zookeeper.forceSync", oldZkForceSync); + else + System.clearProperty("zookeeper.forceSync"); + + if (oldCfgPreprocessor != null) + System.setProperty(GridTestProperties.IGNITE_CFG_PREPROCESSOR_CLS, oldCfgPreprocessor); + else + System.clearProperty(GridTestProperties.IGNITE_CFG_PREPROCESSOR_CLS); + } + + /** {@inheritDoc} */ + @Override protected boolean isMultiJvm() { + return true; + } + + /** {@inheritDoc} */ + @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception { + IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName); + + // Intentionally set TCP discovery in base config. + // The preprocessor must replace it with ZooKeeper discovery. + TcpDiscoverySpi spi = new TcpDiscoverySpi(); + spi.setIpFinder(IP_FINDER); + + cfg.setDiscoverySpi(spi); + + return cfg; + } + + /** {@inheritDoc} */ + @Override protected void afterTest() throws Exception { + stopAllGrids(); + + super.afterTest(); + } + + /** {@inheritDoc} */ + @Override protected Ignite startRemoteGrid( + String igniteInstanceName, + IgniteConfiguration cfg, + GridSpringResourceContext ctx, + boolean resetDiscovery + ) throws Exception { + if (ctx != null) + throw new UnsupportedOperationException( + "Starting a grid in another JVM with Spring context is not supported." + ); + + if (cfg == null) + cfg = optimize(getConfiguration(igniteInstanceName)); + + // Critical for reproducer: + // apply preprocessor before IgniteNodeRunner.storeToFile(...), + // then force resetDiscovery=true to exercise serialization path. + preprocessConfiguration(cfg); Review Comment: > Also I confused with approach, look, there are can be numerous SPI`s and you reset only for TcpDiscoverySpi !! despite of !! boolean resetDiscovery - flag is true `resetDiscovery` is a generic flag, but in the current code path it should only clear the default TcpDiscoverySpi. The reason is that the reproducer first replaces the base TCP discovery with ZookeeperDiscoverySpi, and that non-TCP SPI must survive remote-node serialization. If we reset discovery unconditionally, we lose the already preprocessed ZooKeeper SPI and no longer reproduce the bug. The `instanceof TcpDiscoverySpi` check is intentional. It preserves the existing behavior for the default TCP discovery while preventing accidental removal of a discovery SPI that was already explicitly configured by the test or preprocessor. -- 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]
