Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc merged PR #3629: URL: https://github.com/apache/ignite-3/pull/3629 -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580749014
##
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##
@@ -698,7 +698,9 @@ public CompletableFuture> dataNodes(long
causalityToken, int catalog
);
for (IgniteComponent component : otherComponents) {
-component.start();
+// TODO: Had to remove `willCompleteSuccessfully()` here
Review Comment:
https://issues.apache.org/jira/browse/IGNITE-22119
##
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/distributionzones/ItIgniteDistributionZoneManagerNodeRestartTest.java:
##
@@ -297,7 +301,8 @@ private PartialNode startPartialNode(int idx) {
);
for (IgniteComponent component : otherComponents) {
-component.start();
+// TODO: had to start without waiting as the test otherwise fails
with timeout.
Review Comment:
https://issues.apache.org/jira/browse/IGNITE-22119
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
alievmirza commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580743659
##
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/distributionzones/ItIgniteDistributionZoneManagerNodeRestartTest.java:
##
@@ -297,7 +301,8 @@ private PartialNode startPartialNode(int idx) {
);
for (IgniteComponent component : otherComponents) {
-component.start();
+// TODO: had to start without waiting as the test otherwise fails
with timeout.
Review Comment:
I've created a tickrt
##
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/distributionzones/ItIgniteDistributionZoneManagerNodeRestartTest.java:
##
@@ -297,7 +301,8 @@ private PartialNode startPartialNode(int idx) {
);
for (IgniteComponent component : otherComponents) {
-component.start();
+// TODO: had to start without waiting as the test otherwise fails
with timeout.
Review Comment:
I've created a ticket https://issues.apache.org/jira/browse/IGNITE-22119
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580723266
##
modules/runner/src/main/java/org/apache/ignite/internal/app/LifecycleManager.java:
##
@@ -148,12 +153,11 @@ private synchronized void stopAllComponents() {
}
});
-new ReverseIterator<>(startedComponents).forEachRemaining(component ->
{
-try {
-component.stop();
-} catch (Exception e) {
-LOG.warn("Unable to stop component [component={},
nodeName={}]", e, component, nodeName);
-}
-});
+List> allComponentsStopFuture = new
ArrayList<>();
+
+new ReverseIterator<>(startedComponents).forEachRemaining(component ->
allComponentsStopFuture.add(component.stopAsync()));
+
+allOf(allComponentsStopFuture.toArray(CompletableFuture[]::new))
+.whenComplete((v, e) -> stopFuture.complete(null));
Review Comment:
https://issues.apache.org/jira/browse/IGNITE-22118
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
alievmirza commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580625464
##
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/distributionzones/ItIgniteDistributionZoneManagerNodeRestartTest.java:
##
@@ -297,7 +301,8 @@ private PartialNode startPartialNode(int idx) {
);
for (IgniteComponent component : otherComponents) {
-component.start();
+// TODO: had to start without waiting as the test otherwise fails
with timeout.
Review Comment:
The problems is that in the common node start you have to call
`metaStorageMgr.notifyRevisionUpdateListenerOnStart()`, so some components
(like DZM could proceed with its logic, because it waits for some
VersionedValues)
```
CompletableFuture startupConfigurationUpdate =
notifyConfigurationListeners();
CompletableFuture startupRevisionUpdate =
metaStorageMgr.notifyRevisionUpdateListenerOnStart();
return CompletableFuture.allOf(startupConfigurationUpdate,
startupRevisionUpdate, startFuture)
```
but in this partial node you wait for node to be started without calling
`notifyRevisionUpdateListenerOnStart`, so it can't start. At lease this is the
root cause for `ItIgniteDistributionZoneManagerNodeRestartTest`. I bet the
problem is the same for `ItIgniteNodeRestartTest`
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
alievmirza commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580625464
##
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/distributionzones/ItIgniteDistributionZoneManagerNodeRestartTest.java:
##
@@ -297,7 +301,8 @@ private PartialNode startPartialNode(int idx) {
);
for (IgniteComponent component : otherComponents) {
-component.start();
+// TODO: had to start without waiting as the test otherwise fails
with timeout.
Review Comment:
The problems is that in the common node start you have to call
`metaStorageMgr.notifyRevisionUpdateListenerOnStart()`, so some components
(like DZM could proceed with its logic, because the wait for some
VersionedValues)
```
CompletableFuture startupConfigurationUpdate =
notifyConfigurationListeners();
CompletableFuture startupRevisionUpdate =
metaStorageMgr.notifyRevisionUpdateListenerOnStart();
return CompletableFuture.allOf(startupConfigurationUpdate,
startupRevisionUpdate, startFuture)
```
but in this partial node you wait for node to be started without calling
`notifyRevisionUpdateListenerOnStart`, so it can't start. At lease this is the
root cause for `ItIgniteDistributionZoneManagerNodeRestartTest`. I bet the
problem is the same for `ItIgniteNodeRestartTest`
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580096527
##
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/CatalogTestUtils.java:
##
@@ -91,11 +92,8 @@ public void beforeNodeStop() {
}
@Override
-public void stop() throws Exception {
-super.stop();
-
-clockWaiter.stop();
-metastore.stop();
+public CompletableFuture stopAsync() {
+return allOf(super.stopAsync(), clockWaiter.stopAsync(),
metastore.stopAsync());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580096804
##
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/CatalogTestUtils.java:
##
@@ -126,10 +124,8 @@ public void beforeNodeStop() {
}
@Override
-public void stop() throws Exception {
-super.stop();
-
-metastore.stop();
+public CompletableFuture stopAsync() {
+return allOf(super.stopAsync(), metastore.stopAsync());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580096027
##
modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/ItSimpleCounterServerTest.java:
##
@@ -97,14 +100,12 @@ void before() throws Exception {
server = new JraftServerImpl(service, workDir, raftConfiguration) {
@Override
-public synchronized void stop() throws Exception {
-super.stop();
-
-service.stop();
+public CompletableFuture stopAsync() {
+return CompletableFuture.allOf(super.stopAsync(),
service.stopAsync());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579989020
##
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientHandlerModule.java:
##
@@ -228,10 +230,16 @@ public void stop() throws Exception {
var ch = channel;
if (ch != null) {
-ch.close().await();
+try {
+ch.close().await();
Review Comment:
Not sure. I'd rather not touch this part in this PR, it has already grown
too big.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1580051040
##
modules/runner/src/main/java/org/apache/ignite/internal/app/LifecycleManager.java:
##
@@ -148,12 +153,11 @@ private synchronized void stopAllComponents() {
}
});
-new ReverseIterator<>(startedComponents).forEachRemaining(component ->
{
-try {
-component.stop();
-} catch (Exception e) {
-LOG.warn("Unable to stop component [component={},
nodeName={}]", e, component, nodeName);
-}
-});
+List> allComponentsStopFuture = new
ArrayList<>();
+
+new ReverseIterator<>(startedComponents).forEachRemaining(component ->
allComponentsStopFuture.add(component.stopAsync()));
+
+allOf(allComponentsStopFuture.toArray(CompletableFuture[]::new))
+.whenComplete((v, e) -> stopFuture.complete(null));
Review Comment:
The same applies to start then. I suggest we raise a separate JIRA to cover
that
##
modules/runner/src/main/java/org/apache/ignite/internal/app/LifecycleManager.java:
##
@@ -148,12 +153,11 @@ private synchronized void stopAllComponents() {
}
});
-new ReverseIterator<>(startedComponents).forEachRemaining(component ->
{
-try {
-component.stop();
-} catch (Exception e) {
-LOG.warn("Unable to stop component [component={},
nodeName={}]", e, component, nodeName);
-}
-});
+List> allComponentsStopFuture = new
ArrayList<>();
+
+new ReverseIterator<>(startedComponents).forEachRemaining(component ->
allComponentsStopFuture.add(component.stopAsync()));
+
+allOf(allComponentsStopFuture.toArray(CompletableFuture[]::new))
+.whenComplete((v, e) -> stopFuture.complete(null));
Review Comment:
I suggest we raise a separate JIRA to cover that
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579989020
##
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientHandlerModule.java:
##
@@ -228,10 +230,16 @@ public void stop() throws Exception {
var ch = channel;
if (ch != null) {
-ch.close().await();
+try {
+ch.close().await();
Review Comment:
I'd rather not change the logic in this PR, it has already grown too big.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579986903
##
modules/code-deployment/src/test/java/org/apache/ignite/deployment/metastore/DeploymentUnitStoreImplTest.java:
##
@@ -102,12 +102,12 @@ public void setup() {
ClusterStatusWatchListener clusterListener = new
ClusterStatusWatchListener(clusterEventCallback);
metastore.registerClusterStatusListener(clusterListener);
-metaStorageManager.start();
+assertThat(metaStorageManager.startAsync(),
willCompleteSuccessfully());
toStop = () -> {
nodeListener.stop();
try {
-metaStorageManager.stop();
+assertThat(metaStorageManager.stopAsync(),
willCompleteSuccessfully());
} catch (Exception e) {
Review Comment:
Done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579984730
##
modules/core/src/main/java/org/apache/ignite/internal/manager/IgniteComponent.java:
##
@@ -47,7 +47,7 @@ default void beforeNodeStop() {
* Stops the component. It's guaranteed that during {@code
IgniteComponent#stop())} all components beneath given one are still running,
* however the node is no longer part of the topology and, accordingly,
network interaction is impossible.
*
- * @throws Exception If this component cannot be closed
+ * @return Future that will be completed when the asynchronous part of the
start is processed.
Review Comment:
Done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579982932
##
modules/compute/src/test/java/org/apache/ignite/internal/compute/ComputeComponentImplTest.java:
##
@@ -477,12 +478,12 @@ void remoteExecutionReleasesStopLock() throws Exception {
executeRemotely(SimpleJob.class.getName()).get();
-assertTimeoutPreemptively(Duration.ofSeconds(3), () ->
computeComponent.stop());
+assertTimeoutPreemptively(Duration.ofSeconds(3), () ->
assertThat(computeComponent.stopAsync(), willCompleteSuccessfully()));
}
@Test
void stoppedComponentReturnsExceptionOnExecuteRequestAttempt() throws
Exception {
-computeComponent.stop();
+computeComponent.stopAsync();
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579943253
##
modules/index/src/test/java/org/apache/ignite/internal/index/IndexManagerTest.java:
##
@@ -226,7 +226,10 @@ private void createAndStartComponents() {
lowWatermark
);
-assertThat(allOf(metaStorageManager.start(), catalogManager.start(),
indexManager.start()), willCompleteSuccessfully());
+assertThat(
+allOf(metaStorageManager.startAsync(),
catalogManager.startAsync(), indexManager.startAsync()),
Review Comment:
done
##
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##
@@ -1175,24 +1176,58 @@ public static byte[] byteBufferToByteArray(ByteBuffer
buffer) {
}
}
+private static CompletableFuture startAsync(Stream components) {
+return allOf(components
+.filter(Objects::nonNull)
+.map(IgniteComponent::startAsync)
+.toArray(CompletableFuture[]::new));
+}
+
+/**
+ * Asynchronously starts all ignite components.
+ *
+ * @param components Array of ignite components to start.
+ * @return CompletableFuture that will be completed when all components
are started.
+ */
+public static CompletableFuture startAsync(IgniteComponent...
components) {
+return startAsync(Stream.of(components));
+}
+
+/**
+ * Asynchronously starts all ignite components.
+ *
+ * @param components Collection of ignite components to start.
+ * @return CompletableFuture that will be completed when all components
are started.
+ */
+public static CompletableFuture startAsync(Collection components) {
+return startAsync(components.stream());
+}
+
+private static CompletableFuture stopAsync(Stream components) {
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579939781
##
modules/index/src/test/java/org/apache/ignite/internal/index/IndexManagerTest.java:
##
@@ -123,8 +123,8 @@ public void setUp() {
}
@AfterEach
-void tearDown() throws Exception {
-IgniteUtils.stopAll(metaStorageManager, catalogManager, indexManager);
+void tearDown() {
+assertThat(IgniteUtils.stopAsync(metaStorageManager, catalogManager,
indexManager), willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579938244
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -452,7 +452,13 @@ public synchronized void stop() throws Exception {
Collections.reverse(services);
-IgniteUtils.closeAll(services.stream().map(s -> s::stop));
+try {
+IgniteUtils.closeAll(services.stream().map(s -> s::stop));
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579935936
##
modules/client/src/test/java/org/apache/ignite/client/TestServer.java:
##
@@ -315,10 +317,11 @@ public FakePlacementDriver placementDriver() {
/** {@inheritDoc} */
@Override
public void close() throws Exception {
-module.stop();
-authenticationManager.stop();
-bootstrapFactory.stop();
-cfg.stop();
+assertThat(module.stopAsync(), willCompleteSuccessfully());
+assertThat(authenticationManager.stopAsync(),
willCompleteSuccessfully());
+assertThat(bootstrapFactory.stopAsync(), willCompleteSuccessfully());
+assertThat(cfg.stopAsync(), willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579932468
##
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/BaseDistributionZoneManagerTest.java:
##
@@ -138,11 +134,11 @@ public void tearDown() throws Exception {
Collections.reverse(components);
IgniteUtils.closeAll(components.stream().map(c -> c::beforeNodeStop));
-IgniteUtils.closeAll(components.stream().map(c -> c::stop));
+assertThat(IgniteUtils.stopAsync(components),
willCompleteSuccessfully());
}
void startDistributionZoneManager() {
-assertThat(allOf(distributionZoneManager.start()),
willCompleteSuccessfully());
+assertThat(allOf(distributionZoneManager.startAsync()),
willCompleteSuccessfully());
Review Comment:
fixed
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629: URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579927727 ## modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageManagerImplTest.java: ## @@ -139,9 +139,9 @@ void setUp( metaStorageConfiguration ); -clusterService.start(); -raftManager.start(); -metaStorageManager.start(); +assertThat(clusterService.startAsync(), willCompleteSuccessfully()); Review Comment: done -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579922138
##
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageServiceTest.java:
##
@@ -201,8 +201,8 @@ private static class Node {
}
void start(PeersAndLearners configuration) {
-clusterService.start();
-raftManager.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
+assertThat(raftManager.startAsync(), willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579915557
##
modules/network/src/testFixtures/java/org/apache/ignite/internal/network/utils/ClusterServiceTestUtils.java:
##
@@ -178,24 +177,12 @@ public CompletableFuture start() {
)
).join();
-bootstrapFactory.start();
-
-clusterSvc.start();
-
-return nullCompletedFuture();
+return allOf(bootstrapFactory.startAsync(),
clusterSvc.startAsync());
}
@Override
-public void stop() {
-try {
-IgniteUtils.closeAll(
-clusterSvc::stop,
-bootstrapFactory::stop,
-nodeConfigurationMgr::stop
-);
-} catch (Exception e) {
-throw new RuntimeException(e);
-}
+public CompletableFuture stopAsync() {
+return allOf(clusterSvc.stopAsync(),
bootstrapFactory.stopAsync(), nodeConfigurationMgr.stopAsync());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579912671
##
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/Node.java:
##
@@ -52,13 +55,13 @@ class Node implements AutoCloseable {
}
CompletableFuture startAsync() {
-clusterService.start();
-loza.start();
-metastore.start();
+assertThat(clusterService.startAsync(), willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579905344
##
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##
@@ -209,14 +209,14 @@ private void startPlacementDriverManager() {
new TestClockService(nodeClock)
);
-clusterService.start();
-anotherClusterService.start();
-raftManager.start();
-metaStorageManager.start();
+assertThat(clusterService.startAsync(), willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579903175
##
modules/raft/src/integrationTest/java/org/apache/ignite/internal/raft/ItLearnersTest.java:
##
@@ -120,8 +120,8 @@ Peer asPeer() {
}
void start() {
-clusterService.start();
-loza.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579901568
##
modules/raft/src/integrationTest/java/org/apache/ignite/internal/raft/ItRaftGroupServiceTest.java:
##
@@ -221,8 +221,8 @@ String name() {
}
void start() {
-clusterService.start();
-loza.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579900229
##
modules/raft/src/integrationTest/java/org/apache/ignite/internal/raft/ItRaftGroupServiceTest.java:
##
@@ -258,7 +258,10 @@ void beforeNodeStop() throws Exception {
}
void stop() throws Exception {
-IgniteUtils.closeAll(loza::stop, clusterService::stop);
+IgniteUtils.closeAll(
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629: URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579896240 ## modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/JraftAbstractTest.java: ## @@ -171,7 +173,7 @@ protected JraftServerImpl startServer(int idx, Consumer clo, Consume JraftServerImpl server = jraftServer(servers, idx, service, opts); -server.start(); +server.startAsync(); Review Comment: fixed -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579893056
##
modules/replicator/src/integrationTest/java/org/apache/ignite/internal/replicator/ItPlacementDriverReplicaSideTest.java:
##
@@ -195,19 +197,19 @@ public void beforeTest(TestInfo testInfo) {
replicaManagers.put(nodeName, replicaManager);
-clusterService.start();
-raftManager.start();
-replicaManager.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579892848
##
modules/replicator/src/integrationTest/java/org/apache/ignite/internal/replicator/ItPlacementDriverReplicaSideTest.java:
##
@@ -195,19 +197,19 @@ public void beforeTest(TestInfo testInfo) {
replicaManagers.put(nodeName, replicaManager);
-clusterService.start();
-raftManager.start();
-replicaManager.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
+assertThat(raftManager.startAsync(), willCompleteSuccessfully());
+assertThat(replicaManager.startAsync(),
willCompleteSuccessfully());
servicesToClose.add(() -> {
try {
replicaManager.beforeNodeStop();
raftManager.beforeNodeStop();
clusterService.beforeNodeStop();
-replicaManager.stop();
-raftManager.stop();
-clusterService.stop();
+assertThat(replicaManager.stopAsync(),
willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579887023
##
modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuildingManager.java:
##
@@ -142,20 +143,26 @@ public CompletableFuture start() {
}
@Override
-public void stop() throws Exception {
+public CompletableFuture stopAsync() {
if (!stopGuard.compareAndSet(false, true)) {
-return;
+return nullCompletedFuture();
}
busyLock.block();
-closeAllManually(
-indexBuilder,
-indexAvailabilityController,
-indexBuildController,
-changeIndexStatusTaskController
-);
+try {
+closeAllManually(
+indexBuilder,
+indexAvailabilityController,
+indexBuildController,
+changeIndexStatusTaskController
+);
+} catch (Exception e) {
+return failedFuture(e);
+}
shutdownAndAwaitTermination(executor, 10, TimeUnit.SECONDS);
Review Comment:
Done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579885185
##
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##
@@ -53,18 +53,18 @@ public class DdlCommandHandlerExceptionHandlingTest extends
IgniteAbstractTest {
void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
-assertThat(catalogManager.start(), willCompleteSuccessfully());
+assertThat(catalogManager.startAsync(), willCompleteSuccessfully());
clockWaiter = new ClockWaiter("test", clock);
-assertThat(clockWaiter.start(), willCompleteSuccessfully());
+assertThat(clockWaiter.startAsync(), willCompleteSuccessfully());
commandHandler = new DdlCommandHandler(catalogManager, new
TestClockService(clock, clockWaiter), () -> 100);
}
@AfterEach
public void after() throws Exception {
-clockWaiter.stop();
-catalogManager.stop();
+assertThat(clockWaiter.stopAsync(), willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579880642
##
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##
@@ -1180,16 +1181,24 @@ private RuntimeException handleStartException(Throwable
e) {
LOG.debug(errMsg, e);
-lifecycleManager.stopNode();
+try {
+lifecycleManager.stopNode().get();
+} catch (Exception ex) {
+IgniteException igniteException = new IgniteException(errMsg, ex);
+igniteException.addSuppressed(e);
+
+return igniteException;
+}
return new IgniteException(errMsg, e);
}
/**
* Stops ignite node.
*/
-public void stop() {
-lifecycleManager.stopNode();
+public void stop() throws ExecutionException, InterruptedException {
Review Comment:
Added
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579880267
##
modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuildingManager.java:
##
@@ -142,20 +143,26 @@ public CompletableFuture start() {
}
@Override
-public void stop() throws Exception {
+public CompletableFuture stopAsync() {
if (!stopGuard.compareAndSet(false, true)) {
-return;
+return nullCompletedFuture();
}
busyLock.block();
-closeAllManually(
-indexBuilder,
-indexAvailabilityController,
-indexBuildController,
-changeIndexStatusTaskController
-);
+try {
+closeAllManually(
+indexBuilder,
+indexAvailabilityController,
+indexBuildController,
+changeIndexStatusTaskController
+);
+} catch (Exception e) {
+return failedFuture(e);
+}
shutdownAndAwaitTermination(executor, 10, TimeUnit.SECONDS);
Review Comment:
I'd also include shutdownAndAwaitTermination(executor, ...) into
closeAllManually()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579870204
##
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##
@@ -1180,16 +1181,24 @@ private RuntimeException handleStartException(Throwable
e) {
LOG.debug(errMsg, e);
-lifecycleManager.stopNode();
+try {
+lifecycleManager.stopNode().get();
+} catch (Exception ex) {
+IgniteException igniteException = new IgniteException(errMsg, ex);
+igniteException.addSuppressed(e);
+
+return igniteException;
Review Comment:
Changed
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579867816
##
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##
@@ -53,18 +53,18 @@ public class DdlCommandHandlerExceptionHandlingTest extends
IgniteAbstractTest {
void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
-assertThat(catalogManager.start(), willCompleteSuccessfully());
+assertThat(catalogManager.startAsync(), willCompleteSuccessfully());
clockWaiter = new ClockWaiter("test", clock);
-assertThat(clockWaiter.start(), willCompleteSuccessfully());
+assertThat(clockWaiter.startAsync(), willCompleteSuccessfully());
commandHandler = new DdlCommandHandler(catalogManager, new
TestClockService(clock, clockWaiter), () -> 100);
}
@AfterEach
public void after() throws Exception {
-clockWaiter.stop();
-catalogManager.stop();
+assertThat(clockWaiter.stopAsync(), willCompleteSuccessfully());
Review Comment:
IgniteUtils.stopAsync()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579866759
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -452,7 +452,13 @@ public synchronized void stop() throws Exception {
Collections.reverse(services);
-IgniteUtils.closeAll(services.stream().map(s -> s::stop));
+try {
+IgniteUtils.closeAll(services.stream().map(s -> s::stop));
Review Comment:
static import
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579864902
##
modules/runner/src/main/java/org/apache/ignite/internal/app/LifecycleManager.java:
##
@@ -148,12 +153,11 @@ private synchronized void stopAllComponents() {
}
});
-new ReverseIterator<>(startedComponents).forEachRemaining(component ->
{
-try {
-component.stop();
-} catch (Exception e) {
-LOG.warn("Unable to stop component [component={},
nodeName={}]", e, component, nodeName);
-}
-});
+List> allComponentsStopFuture = new
ArrayList<>();
+
+new ReverseIterator<>(startedComponents).forEachRemaining(component ->
allComponentsStopFuture.add(component.stopAsync()));
+
+allOf(allComponentsStopFuture.toArray(CompletableFuture[]::new))
+.whenComplete((v, e) -> stopFuture.complete(null));
Review Comment:
I'd say that we should explicitly specify pool to execute start/stop,
otherwise we may switch thread unpredictably.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579860866
##
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/BaseCatalogManagerTest.java:
##
@@ -104,19 +102,16 @@ void setUp() {
() ->
CatalogManagerImpl.DEFAULT_PARTITION_IDLE_SAFE_TIME_PROPAGATION_PERIOD
);
-metastore.start();
-clockWaiter.start();
-manager.start();
+assertThat(metastore.startAsync(), willCompleteSuccessfully());
Review Comment:
done
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579859118
##
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##
@@ -1180,16 +1181,24 @@ private RuntimeException handleStartException(Throwable
e) {
LOG.debug(errMsg, e);
-lifecycleManager.stopNode();
+try {
+lifecycleManager.stopNode().get();
+} catch (Exception ex) {
+IgniteException igniteException = new IgniteException(errMsg, ex);
+igniteException.addSuppressed(e);
+
+return igniteException;
Review Comment:
I'd rather return original start exception here. May add stop one as
suppressed.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579859118
##
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##
@@ -1180,16 +1181,24 @@ private RuntimeException handleStartException(Throwable
e) {
LOG.debug(errMsg, e);
-lifecycleManager.stopNode();
+try {
+lifecycleManager.stopNode().get();
+} catch (Exception ex) {
+IgniteException igniteException = new IgniteException(errMsg, ex);
+igniteException.addSuppressed(e);
+
+return igniteException;
Review Comment:
I'd rather return original start exception here.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579857581
##
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##
@@ -1180,16 +1181,24 @@ private RuntimeException handleStartException(Throwable
e) {
LOG.debug(errMsg, e);
-lifecycleManager.stopNode();
+try {
+lifecycleManager.stopNode().get();
+} catch (Exception ex) {
+IgniteException igniteException = new IgniteException(errMsg, ex);
+igniteException.addSuppressed(e);
+
+return igniteException;
+}
return new IgniteException(errMsg, e);
}
/**
* Stops ignite node.
*/
-public void stop() {
-lifecycleManager.stopNode();
+public void stop() throws ExecutionException, InterruptedException {
Review Comment:
I'd say that we should provide both stopAsync() an stop() in order to be
consistent.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579857130
##
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerRecoveryTest.java:
##
@@ -81,8 +81,8 @@ public class CatalogManagerRecoveryTest extends
BaseIgniteAbstractTest {
private TestUpdateHandlerInterceptor interceptor;
@AfterEach
-void tearDown() throws Exception {
-IgniteUtils.stopAll(catalogManager, metaStorageManager);
+void tearDown() {
+assertThat(IgniteUtils.stopAsync(catalogManager, metaStorageManager),
willCompleteSuccessfully());
Review Comment:
Added exception handler in stopAsync
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579846730
##
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##
@@ -698,7 +698,9 @@ public CompletableFuture> dataNodes(long
causalityToken, int catalog
);
for (IgniteComponent component : otherComponents) {
-component.start();
+// TODO: Had to remove `willCompleteSuccessfully()` here
Review Comment:
@alievmirza
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579840663
##
modules/replicator/src/integrationTest/java/org/apache/ignite/internal/replicator/ItPlacementDriverReplicaSideTest.java:
##
@@ -195,19 +197,19 @@ public void beforeTest(TestInfo testInfo) {
replicaManagers.put(nodeName, replicaManager);
-clusterService.start();
-raftManager.start();
-replicaManager.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
Review Comment:
IgniteUtils.startAsync()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579841013
##
modules/replicator/src/integrationTest/java/org/apache/ignite/internal/replicator/ItPlacementDriverReplicaSideTest.java:
##
@@ -195,19 +197,19 @@ public void beforeTest(TestInfo testInfo) {
replicaManagers.put(nodeName, replicaManager);
-clusterService.start();
-raftManager.start();
-replicaManager.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
+assertThat(raftManager.startAsync(), willCompleteSuccessfully());
+assertThat(replicaManager.startAsync(),
willCompleteSuccessfully());
servicesToClose.add(() -> {
try {
replicaManager.beforeNodeStop();
raftManager.beforeNodeStop();
clusterService.beforeNodeStop();
-replicaManager.stop();
-raftManager.stop();
-clusterService.stop();
+assertThat(replicaManager.stopAsync(),
willCompleteSuccessfully());
Review Comment:
IgniteUtils.stopAsync()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579840663
##
modules/replicator/src/integrationTest/java/org/apache/ignite/internal/replicator/ItPlacementDriverReplicaSideTest.java:
##
@@ -195,19 +197,19 @@ public void beforeTest(TestInfo testInfo) {
replicaManagers.put(nodeName, replicaManager);
-clusterService.start();
-raftManager.start();
-replicaManager.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
Review Comment:
IgniteUtils.stopAsync()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629: URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579824596 ## modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/JraftAbstractTest.java: ## @@ -171,7 +173,7 @@ protected JraftServerImpl startServer(int idx, Consumer clo, Consume JraftServerImpl server = jraftServer(servers, idx, service, opts); -server.start(); +server.startAsync(); Review Comment: assertion? -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579823834
##
modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/ItSimpleCounterServerTest.java:
##
@@ -97,14 +100,12 @@ void before() throws Exception {
server = new JraftServerImpl(service, workDir, raftConfiguration) {
@Override
-public synchronized void stop() throws Exception {
-super.stop();
-
-service.stop();
+public CompletableFuture stopAsync() {
+return CompletableFuture.allOf(super.stopAsync(),
service.stopAsync());
Review Comment:
As usual IgniteUtils.stopAsyn()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579729570
##
modules/raft/src/integrationTest/java/org/apache/ignite/internal/raft/ItRaftGroupServiceTest.java:
##
@@ -258,7 +258,10 @@ void beforeNodeStop() throws Exception {
}
void stop() throws Exception {
-IgniteUtils.closeAll(loza::stop, clusterService::stop);
+IgniteUtils.closeAll(
Review Comment:
IgniteUtils.stopAsync()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579727992
##
modules/raft/src/integrationTest/java/org/apache/ignite/internal/raft/ItLearnersTest.java:
##
@@ -120,8 +120,8 @@ Peer asPeer() {
}
void start() {
-clusterService.start();
-loza.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
Review Comment:
IgniteUtils.startAsync()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579728997
##
modules/raft/src/integrationTest/java/org/apache/ignite/internal/raft/ItRaftGroupServiceTest.java:
##
@@ -221,8 +221,8 @@ String name() {
}
void start() {
-clusterService.start();
-loza.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
Review Comment:
IgniteUtils.startAsync()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579724921
##
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##
@@ -209,14 +209,14 @@ private void startPlacementDriverManager() {
new TestClockService(nodeClock)
);
-clusterService.start();
-anotherClusterService.start();
-raftManager.start();
-metaStorageManager.start();
+assertThat(clusterService.startAsync(), willCompleteSuccessfully());
Review Comment:
IgniteUtils.startAsync()
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579724136
##
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/Node.java:
##
@@ -52,13 +55,13 @@ class Node implements AutoCloseable {
}
CompletableFuture startAsync() {
-clusterService.start();
-loza.start();
-metastore.start();
+assertThat(clusterService.startAsync(), willCompleteSuccessfully());
Review Comment:
IgniteUtils.startAsync
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579723031
##
modules/network/src/testFixtures/java/org/apache/ignite/internal/network/utils/ClusterServiceTestUtils.java:
##
@@ -178,24 +177,12 @@ public CompletableFuture start() {
)
).join();
-bootstrapFactory.start();
-
-clusterSvc.start();
-
-return nullCompletedFuture();
+return allOf(bootstrapFactory.startAsync(),
clusterSvc.startAsync());
}
@Override
-public void stop() {
-try {
-IgniteUtils.closeAll(
-clusterSvc::stop,
-bootstrapFactory::stop,
-nodeConfigurationMgr::stop
-);
-} catch (Exception e) {
-throw new RuntimeException(e);
-}
+public CompletableFuture stopAsync() {
+return allOf(clusterSvc.stopAsync(),
bootstrapFactory.stopAsync(), nodeConfigurationMgr.stopAsync());
Review Comment:
It's not an equivalent of the original code. closeAll tolerates exceptions
while closing each resource and thus best-effort.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579703433
##
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageServiceTest.java:
##
@@ -201,8 +201,8 @@ private static class Node {
}
void start(PeersAndLearners configuration) {
-clusterService.start();
-raftManager.start();
+assertThat(clusterService.startAsync(),
willCompleteSuccessfully());
+assertThat(raftManager.startAsync(), willCompleteSuccessfully());
Review Comment:
IgniteUtils.startAsync
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579701159
##
modules/index/src/test/java/org/apache/ignite/internal/index/IndexManagerTest.java:
##
@@ -123,8 +123,8 @@ public void setUp() {
}
@AfterEach
-void tearDown() throws Exception {
-IgniteUtils.stopAll(metaStorageManager, catalogManager, indexManager);
+void tearDown() {
+assertThat(IgniteUtils.stopAsync(metaStorageManager, catalogManager,
indexManager), willCompleteSuccessfully());
Review Comment:
Here and there. Is it possible to use static imports for IgniteUtils or
there's clashing?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629: URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579697076 ## modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageManagerImplTest.java: ## @@ -139,9 +139,9 @@ void setUp( metaStorageConfiguration ); -clusterService.start(); -raftManager.start(); -metaStorageManager.start(); +assertThat(clusterService.startAsync(), willCompleteSuccessfully()); Review Comment: startAsync -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629: URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579697076 ## modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageManagerImplTest.java: ## @@ -139,9 +139,9 @@ void setUp( metaStorageConfiguration ); -clusterService.start(); -raftManager.start(); -metaStorageManager.start(); +assertThat(clusterService.startAsync(), willCompleteSuccessfully()); Review Comment: IgniteUtils.startAsync(clusterService, raftManager, ...) -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579694364
##
modules/index/src/test/java/org/apache/ignite/internal/index/IndexManagerTest.java:
##
@@ -226,7 +226,10 @@ private void createAndStartComponents() {
lowWatermark
);
-assertThat(allOf(metaStorageManager.start(), catalogManager.start(),
indexManager.start()), willCompleteSuccessfully());
+assertThat(
+allOf(metaStorageManager.startAsync(),
catalogManager.startAsync(), indexManager.startAsync()),
Review Comment:
Here and there, should we use startAsync() that you've introduced in such
places?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579654791
##
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/BaseDistributionZoneManagerTest.java:
##
@@ -138,11 +134,11 @@ public void tearDown() throws Exception {
Collections.reverse(components);
IgniteUtils.closeAll(components.stream().map(c -> c::beforeNodeStop));
-IgniteUtils.closeAll(components.stream().map(c -> c::stop));
+assertThat(IgniteUtils.stopAsync(components),
willCompleteSuccessfully());
}
void startDistributionZoneManager() {
-assertThat(allOf(distributionZoneManager.start()),
willCompleteSuccessfully());
+assertThat(allOf(distributionZoneManager.startAsync()),
willCompleteSuccessfully());
Review Comment:
allOf single))
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579648035
##
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/distributionzones/ItIgniteDistributionZoneManagerNodeRestartTest.java:
##
@@ -297,7 +301,8 @@ private PartialNode startPartialNode(int idx) {
);
for (IgniteComponent component : otherComponents) {
-component.start();
+// TODO: had to start without waiting as the test otherwise fails
with timeout.
Review Comment:
Please don't forget to remove todo, after receiving the feedback from
@alievmirza
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579640666
##
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##
@@ -1175,24 +1176,58 @@ public static byte[] byteBufferToByteArray(ByteBuffer
buffer) {
}
}
+private static CompletableFuture startAsync(Stream components) {
+return allOf(components
+.filter(Objects::nonNull)
+.map(IgniteComponent::startAsync)
+.toArray(CompletableFuture[]::new));
+}
+
+/**
+ * Asynchronously starts all ignite components.
+ *
+ * @param components Array of ignite components to start.
+ * @return CompletableFuture that will be completed when all components
are started.
+ */
+public static CompletableFuture startAsync(IgniteComponent...
components) {
+return startAsync(Stream.of(components));
+}
+
+/**
+ * Asynchronously starts all ignite components.
+ *
+ * @param components Collection of ignite components to start.
+ * @return CompletableFuture that will be completed when all components
are started.
+ */
+public static CompletableFuture startAsync(Collection components) {
+return startAsync(components.stream());
+}
+
+private static CompletableFuture stopAsync(Stream components) {
Review Comment:
Please check my comment in
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerRecoveryTest.java
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579636368
##
modules/core/src/main/java/org/apache/ignite/internal/manager/IgniteComponent.java:
##
@@ -47,7 +47,7 @@ default void beforeNodeStop() {
* Stops the component. It's guaranteed that during {@code
IgniteComponent#stop())} all components beneath given one are still running,
* however the node is no longer part of the topology and, accordingly,
network interaction is impossible.
*
- * @throws Exception If this component cannot be closed
+ * @return Future that will be completed when the asynchronous part of the
start is processed.
Review Comment:
stop instead of start.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579617708
##
modules/compute/src/test/java/org/apache/ignite/internal/compute/ComputeComponentImplTest.java:
##
@@ -477,12 +478,12 @@ void remoteExecutionReleasesStopLock() throws Exception {
executeRemotely(SimpleJob.class.getName()).get();
-assertTimeoutPreemptively(Duration.ofSeconds(3), () ->
computeComponent.stop());
+assertTimeoutPreemptively(Duration.ofSeconds(3), () ->
assertThat(computeComponent.stopAsync(), willCompleteSuccessfully()));
}
@Test
void stoppedComponentReturnsExceptionOnExecuteRequestAttempt() throws
Exception {
-computeComponent.stop();
+computeComponent.stopAsync();
Review Comment:
assertion is missing.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579605232
##
modules/code-deployment/src/test/java/org/apache/ignite/deployment/metastore/DeploymentUnitStoreImplTest.java:
##
@@ -102,12 +102,12 @@ public void setup() {
ClusterStatusWatchListener clusterListener = new
ClusterStatusWatchListener(clusterEventCallback);
metastore.registerClusterStatusListener(clusterListener);
-metaStorageManager.start();
+assertThat(metaStorageManager.startAsync(),
willCompleteSuccessfully());
toStop = () -> {
nodeListener.stop();
try {
-metaStorageManager.stop();
+assertThat(metaStorageManager.stopAsync(),
willCompleteSuccessfully());
} catch (Exception e) {
Review Comment:
Here and there, stopAsync no longer throws Exception, thus no need to catch
it.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579578818
##
modules/client/src/test/java/org/apache/ignite/client/TestServer.java:
##
@@ -315,10 +317,11 @@ public FakePlacementDriver placementDriver() {
/** {@inheritDoc} */
@Override
public void close() throws Exception {
-module.stop();
-authenticationManager.stop();
-bootstrapFactory.stop();
-cfg.stop();
+assertThat(module.stopAsync(), willCompleteSuccessfully());
+assertThat(authenticationManager.stopAsync(),
willCompleteSuccessfully());
+assertThat(bootstrapFactory.stopAsync(), willCompleteSuccessfully());
+assertThat(cfg.stopAsync(), willCompleteSuccessfully());
Review Comment:
Same as above (won't repeat myself furthermore), allOf would be better here.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579572557
##
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientHandlerModule.java:
##
@@ -228,10 +230,16 @@ public void stop() throws Exception {
var ch = channel;
if (ch != null) {
-ch.close().await();
+try {
+ch.close().await();
Review Comment:
Is await is a sort of join?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579528313
##
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/CatalogTestUtils.java:
##
@@ -126,10 +124,8 @@ public void beforeNodeStop() {
}
@Override
-public void stop() throws Exception {
-super.stop();
-
-metastore.stop();
+public CompletableFuture stopAsync() {
+return allOf(super.stopAsync(), metastore.stopAsync());
Review Comment:
Same as above. Basically there are plenty of places within tests where it's
better to use best-effort stop logic.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579524843
##
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/CatalogTestUtils.java:
##
@@ -91,11 +92,8 @@ public void beforeNodeStop() {
}
@Override
-public void stop() throws Exception {
-super.stop();
-
-clockWaiter.stop();
-metastore.stop();
+public CompletableFuture stopAsync() {
+return allOf(super.stopAsync(), clockWaiter.stopAsync(),
metastore.stopAsync());
Review Comment:
I'd rather use IgniteUtils.stopAsync here.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579522019
##
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/BaseCatalogManagerTest.java:
##
@@ -104,19 +102,16 @@ void setUp() {
() ->
CatalogManagerImpl.DEFAULT_PARTITION_IDLE_SAFE_TIME_PROPAGATION_PERIOD
);
-metastore.start();
-clockWaiter.start();
-manager.start();
+assertThat(metastore.startAsync(), willCompleteSuccessfully());
Review Comment:
I'd rather have allOf here with willCompleteSuccessfully. Otherwise our
async start becomes effectively sync.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1579468084
##
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerRecoveryTest.java:
##
@@ -81,8 +81,8 @@ public class CatalogManagerRecoveryTest extends
BaseIgniteAbstractTest {
private TestUpdateHandlerInterceptor interceptor;
@AfterEach
-void tearDown() throws Exception {
-IgniteUtils.stopAll(catalogManager, metaStorageManager);
+void tearDown() {
+assertThat(IgniteUtils.stopAsync(catalogManager, metaStorageManager),
willCompleteSuccessfully());
Review Comment:
stopAsync according to how it's implemented now is not an equivalent of an
original stopAll that 've used closeAll that on its turn proceeds with
components stopping even in case of exception thrown from the synchronous part
of the stop.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576570458
##
modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/ItSimpleCounterServerTest.java:
##
@@ -97,14 +99,16 @@ void before() throws Exception {
server = new JraftServerImpl(service, workDir, raftConfiguration) {
@Override
-public synchronized void stop() throws Exception {
-super.stop();
+public synchronized CompletableFuture stopAsync() {
Review Comment:
Looks like it's a leftover from the past times. removed it.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576566401
##
modules/network/src/testFixtures/java/org/apache/ignite/internal/network/utils/ClusterServiceTestUtils.java:
##
@@ -178,24 +179,26 @@ public CompletableFuture start() {
)
).join();
-bootstrapFactory.start();
+bootstrapFactory.startAsync();
-clusterSvc.start();
+clusterSvc.startAsync();
return nullCompletedFuture();
}
@Override
-public void stop() {
+public CompletableFuture stopAsync() {
try {
IgniteUtils.closeAll(
Review Comment:
fixed
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576566038
##
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##
@@ -239,9 +239,9 @@ public void testNoInteractionsAfterStop() throws Exception {
CompletableFuture readyFuture = manager.catalogReadyFuture(1);
assertFalse(readyFuture.isDone());
-manager.stop();
+manager.stopAsync();
Review Comment:
fixed
##
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogTestUtilsTest.java:
##
@@ -80,7 +80,7 @@ void testManagerWorksAsExpected() throws Exception {
assertThat(tablesOfVersion2, hasItem(descriptorWithName("T1")));
assertThat(tablesOfVersion2, hasItem(descriptorWithName("T2")));
-manager.stop();
+manager.stopAsync();
Review Comment:
fixed
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
Cyrill commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576565805
##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##
@@ -191,16 +191,18 @@ public CompletableFuture start() {
updateLog.registerUpdateHandler(new OnUpdateHandlerImpl());
-updateLog.start();
+updateLog.startAsync();
return nullCompletedFuture();
}
@Override
-public void stop() throws Exception {
+public CompletableFuture stopAsync() {
busyLock.block();
versionTracker.close();
-updateLog.stop();
+updateLog.stopAsync();
Review Comment:
fixed
##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##
@@ -191,16 +191,18 @@ public CompletableFuture start() {
updateLog.registerUpdateHandler(new OnUpdateHandlerImpl());
-updateLog.start();
+updateLog.startAsync();
Review Comment:
fixed
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576130991
##
modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/ItSimpleCounterServerTest.java:
##
@@ -97,14 +99,16 @@ void before() throws Exception {
server = new JraftServerImpl(service, workDir, raftConfiguration) {
@Override
-public synchronized void stop() throws Exception {
-super.stop();
+public synchronized CompletableFuture stopAsync() {
Review Comment:
Curious whether synchronized was expected to cover all stop actions?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576119948
##
modules/network/src/testFixtures/java/org/apache/ignite/internal/network/utils/ClusterServiceTestUtils.java:
##
@@ -178,24 +179,26 @@ public CompletableFuture start() {
)
).join();
-bootstrapFactory.start();
+bootstrapFactory.startAsync();
-clusterSvc.start();
+clusterSvc.startAsync();
return nullCompletedFuture();
}
@Override
-public void stop() {
+public CompletableFuture stopAsync() {
try {
IgniteUtils.closeAll(
Review Comment:
How are we going to handle async exceptions inside closeAll? I mean that it
was designed to close as much as possible, semi-ignoring pending exceptions.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576029691
##
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogTestUtilsTest.java:
##
@@ -80,7 +80,7 @@ void testManagerWorksAsExpected() throws Exception {
assertThat(tablesOfVersion2, hasItem(descriptorWithName("T1")));
assertThat(tablesOfVersion2, hasItem(descriptorWithName("T2")));
-manager.stop();
+manager.stopAsync();
Review Comment:
Same as above, won't repeat myself below, please add verification that
corresponding future was completed successfully.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-22071 Async component stop [ignite-3]
sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576010330
##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##
@@ -191,16 +191,18 @@ public CompletableFuture start() {
updateLog.registerUpdateHandler(new OnUpdateHandlerImpl());
-updateLog.start();
+updateLog.startAsync();
Review Comment:
That's not part of your PR actually, but anyway, we should not ignore
`updateLog.startAsync();` future. Proper async postfix 've made it visible.
##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##
@@ -191,16 +191,18 @@ public CompletableFuture start() {
updateLog.registerUpdateHandler(new OnUpdateHandlerImpl());
-updateLog.start();
+updateLog.startAsync();
return nullCompletedFuture();
}
@Override
-public void stop() throws Exception {
+public CompletableFuture stopAsync() {
busyLock.block();
versionTracker.close();
-updateLog.stop();
+updateLog.stopAsync();
Review Comment:
Basically same as above, that doesn't look right. Why updateLog.stopAsync()
future is ignored?
##
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##
@@ -239,9 +239,9 @@ public void testNoInteractionsAfterStop() throws Exception {
CompletableFuture readyFuture = manager.catalogReadyFuture(1);
assertFalse(readyFuture.isDone());
-manager.stop();
+manager.stopAsync();
Review Comment:
Here and in other places, let's add verification that corresponding
startAsync/stopAsync futures were completed successfully. I'm talking about
`assertThat(manager.stopAsync(), willCompleteSuccessfully());` In some places
we may combine components start/stop futures and assert that combined one was
completed successfully - not mandatory action though.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
