tkalkirill commented on code in PR #4239: URL: https://github.com/apache/ignite-3/pull/4239#discussion_r1723343806
########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/app/ItInProcessRestartTest.java: ########## @@ -0,0 +1,95 @@ +/* + * 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.app; + +import static java.util.concurrent.CompletableFuture.runAsync; +import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.ignite.Ignite; +import org.apache.ignite.internal.ClusterPerTestIntegrationTest; +import org.apache.ignite.internal.testframework.WorkDirectoryExtension; +import org.apache.ignite.lang.NodeNotStartedException; +import org.apache.ignite.table.KeyValueView; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +@ExtendWith(WorkDirectoryExtension.class) +class ItInProcessRestartTest extends ClusterPerTestIntegrationTest { + @Override + protected int initialNodes() { + return 1; + } + + @Test + void restarts() { + IgniteServerImpl server = (IgniteServerImpl) cluster.server(0); + + assertThat(server.restartAsync(), willCompleteSuccessfully()); + } + + @Test + void restartAfterShutdownThrows() { + IgniteServerImpl server = (IgniteServerImpl) cluster.server(0); + + server.shutdown(); + + assertThrows(NodeNotStartedException.class, server::restartAsync); + } + + /** + * Makes sure that operations happening during a restart finish successfully. + */ + @Test + void restartIsTransparent() { + Ignite ignite = node(0); + + ignite.sql().executeScript("CREATE TABLE test (id INT PRIMARY KEY, val VARCHAR)"); + KeyValueView<Integer, String> kvView = ignite.tables().table("test").keyValueView(Integer.class, String.class); + + var insertedSomething = new CompletableFuture<Void>(); + + AtomicBoolean restarted = new AtomicBoolean(false); Review Comment: Optional: you can also use var for these variables. ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/app/SyncApiOperation.java: ########## @@ -0,0 +1,117 @@ +/* + * 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.app; + +import static org.apache.ignite.internal.app.ApiReferencesTestUtils.FULL_TUPLE; +import static org.apache.ignite.internal.app.ApiReferencesTestUtils.KEY_TUPLE; +import static org.apache.ignite.internal.app.ApiReferencesTestUtils.TEST_TABLE_NAME; +import static org.apache.ignite.internal.app.ApiReferencesTestUtils.VALUE_TUPLE; + +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; +import org.apache.ignite.table.mapper.Mapper; + +/** + * Synchronous API operation. + */ +@SuppressWarnings("resource") +enum SyncApiOperation { + IGNITE_NAME(refs -> refs.ignite.name()), Review Comment: I would simplify it, at your discretion. ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/app/Record.java: ########## @@ -0,0 +1,45 @@ +/* + * 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.app; + +import org.apache.ignite.table.Tuple; + +/** + * Simple record for tests. + */ +@SuppressWarnings({"FieldCanBeLocal", "unused"}) +class Record { + private int id; + private String val; Review Comment: Thanks. ########## modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteServerImpl.java: ########## @@ -188,13 +236,64 @@ public CompletableFuture<Void> waitForInitAsync() { return joinFuture; } + /** + * Restarts the node asynchronously. The {@link Ignite} instance obtained via {@link #api()} and objects acquired through it remain + * functional, but completion of calls to them might be delayed during the restart (that is, synchronous calls might take more time, + * while futures from asynchronous calls might take more time to complete). + * + * @return CompletableFuture that gets completed when the node startup has completed (either successfully or with an error). + */ + CompletableFuture<Void> restartAsync() { Review Comment: It looks more complicated to me, but it's up to you. ########## modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteServerImpl.java: ########## @@ -82,10 +86,33 @@ public class IgniteServerImpl implements IgniteServer { private final ClassLoader classLoader; - private volatile @Nullable IgniteImpl instance; + private final Executor asyncContinuationExecutor; + + /** Current Ignite instance. This field is not volatile to make hot path accesses from IgniteReference and other references + * faster (they always happen under a read lock, which guarantees visibility of changes to this field). So we access + * this field in this object under synchronization ({@link #igniteMonitor} serves as the monitor). + */ + private @Nullable IgniteImpl ignite; Review Comment: If I understood the documentation correctly, it says that reading from the synchronization block is cheaper in terms of performance than one volatile read. Is there any proof of this? If we have two parallel threads reading this field, they will step on each other's toes and park someone, which will lead to performance degradation. ########## modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteServerImpl.java: ########## @@ -281,9 +386,11 @@ private static void ackBanner() { private static void sync(CompletableFuture<Void> future) { try { - future.join(); - } catch (CompletionException e) { + future.get(); Review Comment: Fair. ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/app/ItShutDownServerApiReferencesTest.java: ########## @@ -0,0 +1,70 @@ +/* + * 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.app; + +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.apache.ignite.internal.testframework.asserts.CompletableFutureAssert.assertWillThrow; +import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.apache.ignite.internal.ClusterPerClassIntegrationTest; +import org.apache.ignite.lang.IgniteException; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +/** + * Makes sure that references to API objects obtained in embedded mode stop functioning after the node gets shut down. + */ +class ItShutDownServerApiReferencesTest extends ClusterPerClassIntegrationTest { + private static IgniteServerImpl server; + + private static References beforeShutdown; + + @Override + protected int initialNodes() { + return 1; + } + + @BeforeAll + void init() throws Exception { + server = (IgniteServerImpl) CLUSTER.server(0); + + server.api().sql().executeScript("CREATE TABLE test (id INT PRIMARY KEY, val VARCHAR)"); + + beforeShutdown = new References(server); + + assertThat(server.shutdownAsync(), willCompleteSuccessfully()); + } + + @ParameterizedTest + @EnumSource(SyncApiOperation.class) + void syncOperationsThrowAfterShutdown(SyncApiOperation operation) { + IgniteException ex = assertThrows(IgniteException.class, () -> operation.execute(beforeShutdown)); Review Comment: It looks a little more convenient, you can send a fragment of a message there, at your discretion. ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/app/SyncApiOperation.java: ########## @@ -0,0 +1,117 @@ +/* + * 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.app; + +import static org.apache.ignite.internal.app.ApiReferencesTestUtils.FULL_TUPLE; +import static org.apache.ignite.internal.app.ApiReferencesTestUtils.KEY_TUPLE; +import static org.apache.ignite.internal.app.ApiReferencesTestUtils.TEST_TABLE_NAME; +import static org.apache.ignite.internal.app.ApiReferencesTestUtils.VALUE_TUPLE; + +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; +import org.apache.ignite.table.mapper.Mapper; + +/** + * Synchronous API operation. + */ +@SuppressWarnings("resource") +enum SyncApiOperation { + IGNITE_NAME(refs -> refs.ignite.name()), + IGNITE_TABLES(refs -> refs.ignite.tables()), + IGNITE_TRANSACTIONS(refs -> refs.ignite.transactions()), + IGNITE_SQL(refs -> refs.ignite.sql()), + IGNITE_COMPUTE(refs -> refs.ignite.compute()), + IGNITE_CLUSTER_NODES(refs -> refs.ignite.clusterNodes()), + IGNITE_CATALOG(refs -> refs.ignite.catalog()), + + TABLES_TABLES(refs -> refs.tables.tables()), + TABLES_TABLE(refs -> refs.tables.table(TEST_TABLE_NAME)), + + TABLE_NAME(refs -> refs.table.name()), + TABLE_KVVIEW(refs -> refs.table.keyValueView()), + TABLE_TYPED_KVVIEW(refs -> refs.table.keyValueView(Integer.class, String.class)), + TABLE_MAPPED_KVVIEW(refs -> refs.table.keyValueView(Mapper.of(Integer.class), Mapper.of(String.class))), + TABLE_RECORDVIEW(refs -> refs.table.recordView()), + TABLE_TYPED_RECORDVIEW(refs -> refs.table.recordView(Record.class)), + TABLE_MAPPED_RECORDVIEW(refs -> refs.table.recordView(Mapper.of(Record.class))), + TABLE_PARTITION_MANAGER(refs -> refs.table.partitionManager()), + + TABLE_FROM_TABLE_ASYNC_PUT(refs -> refs.tableFromTableAsync.keyValueView().put(null, KEY_TUPLE, VALUE_TUPLE)), Review Comment: Ok ########## modules/runner/src/main/java/org/apache/ignite/internal/restart/RestartProofApiObject.java: ########## @@ -0,0 +1,71 @@ +/* + * 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.restart; + +import java.util.concurrent.CompletableFuture; +import java.util.function.Consumer; +import java.util.function.Function; +import org.apache.ignite.Ignite; + +/** + * Base for references to API objects under a swappable {@link Ignite}. + */ +abstract class RestartProofApiObject<T> { Review Comment: Ok -- 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]
