You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "nevdmitry (via GitHub)" <gi...@apache.org> on 2023/07/25 15:51:48 UTC
[GitHub] [ignite-3] nevdmitry opened a new pull request, #2359: IGNITE-19108 Check connection during REPL session in CLI
nevdmitry opened a new pull request, #2359:
URL: https://github.com/apache/ignite-3/pull/2359
https://issues.apache.org/jira/browse/IGNITE-19108
- start connection heartbeat thread on 'connect' command
- stop heartbeat on 'disconnect'
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] nevdmitry commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "nevdmitry (via GitHub)" <gi...@apache.org>.
nevdmitry commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1284706708
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
+ this.startNode(nodeName);
Review Comment:
it is some kind of tear down. Restore node state which was started in the begining of test
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1287218708
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/AsyncConnectionEventListener.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.cli.event;
+
+import org.apache.ignite.internal.cli.core.repl.ConnectEvent;
+import org.apache.ignite.internal.cli.core.repl.SessionInfo;
+
+/**
+ * Listener of any connection related events.
+ */
+public abstract class AsyncConnectionEventListener implements EventListener {
Review Comment:
Also, it can should be interface with default methods.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] nevdmitry commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "nevdmitry (via GitHub)" <gi...@apache.org>.
nevdmitry commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1278820315
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/connect/ConnectCall.java:
##########
@@ -81,8 +86,8 @@ public CallOutput<String> execute(UrlCallInput input) {
stateConfigProvider.get().setProperty(CliConfigKeys.LAST_CONNECTED_URL.value(), nodeUrl);
String jdbcUrl = jdbcUrlFactory.constructJdbcUrl(configuration, nodeUrl);
- session.connect(new SessionInfo(nodeUrl, fetchNodeName(nodeUrl), jdbcUrl, username));
-
+ session.connect(new SessionInfo(nodeUrl, fetchNodeName(nodeUrl), jdbcUrl, username, ConnectionStatus.OPEN));
+ connectionHeartBeat.start(session, clientFactory);
Review Comment:
it is good idea but due to some circular dependencies between Session and ReplPromtProvider I leave it as is
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1279410035
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.cli.core.repl;
+
+import jakarta.inject.Singleton;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat {
+
+ private static final IgniteLogger log = CliLoggers.forClass(ConnectionHeartBeat.class);
+
+ /** CLI check connection period period. */
+ public static long CLI_CHECK_CONNECTION_PERIOD_SECONDS = 5;
+
+ /** Scheduled executor for connection heartbeat. */
+ @Nullable
+ private ScheduledExecutorService scheduledConnectionHeartbeatExecutor;
+
+ private final ApiClientFactory clientFactory;
+
+ private final List<? extends AsyncConnectionEventListener> listeners;
+
+ private final AtomicBoolean connected = new AtomicBoolean(false);
+
+ public ConnectionHeartBeat(ApiClientFactory clientFactory, List<? extends AsyncConnectionEventListener> listeners) {
+ this.clientFactory = clientFactory;
+ this.listeners = listeners;
+ }
+
+ /**
+ * Starts connection heartbeat. By default connection will be checked every 5 sec.
+ *
+ * @param sessionInfo session info with node url
+ */
+ public void start(SessionInfo sessionInfo) {
+ connectionEstablished();
+
+ if (scheduledConnectionHeartbeatExecutor == null) {
+ scheduledConnectionHeartbeatExecutor =
+ Executors.newScheduledThreadPool(1, new NamedThreadFactory("cli-check-connection-thread", log));
+
+ //Start connection heart beat
+ scheduledConnectionHeartbeatExecutor.scheduleAtFixedRate(
+ () -> pingConnection(sessionInfo.nodeUrl()),
+ 0,
+ CLI_CHECK_CONNECTION_PERIOD_SECONDS,
+ TimeUnit.SECONDS
+ );
+ }
+ }
+
+ /**
+ * Stops connection heartbeat.
+ */
+ public void stop() {
Review Comment:
Can you make start/stop methods thread safe? I think synchronized will be ok.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.cli.commands;
+
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.after;
+import static org.mockito.Mockito.verify;
+
+import jakarta.inject.Inject;
+import org.apache.ignite.IgnitionManager;
+import org.apache.ignite.internal.cli.core.repl.ConnectionHeartBeat;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.core.repl.prompt.ReplPromptProvider;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Spy;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Spy
+ @Inject
+ private ReplPromptProvider replPromptProvider;
+
+ @BeforeEach
+ void setUp() {
+ //Set connection check timeout to 1 sec to make test fast
+ ConnectionHeartBeat.CLI_CHECK_CONNECTION_PERIOD_SECONDS = 1L;
+ }
+
+ @AfterEach
+ void teardown() {
+ // Start all nodes
+ allNodeNames().forEach(this::startNode);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should invoke onConnection() on connection established")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ verify(replPromptProvider, after(ConnectionHeartBeat.CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2L).times(1)).onConnection();
+ verify(replPromptProvider, after(ConnectionHeartBeat.CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2L).never()).onConnectionLost();
+ }
+
+ @Test
+ @DisplayName("Should invoke onConnectionLost()")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop all nodes
+ allNodeNames().forEach(IgnitionManager::stop);
+
+ verify(replPromptProvider, after(ConnectionHeartBeat.CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2L).times(1)).onConnectionLost();
Review Comment:
It would be safer to use the `Awaitility` library here and above. Something can go wrong on CI and we'll get one more flaky test.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.cli.commands;
+
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.after;
+import static org.mockito.Mockito.verify;
+
+import jakarta.inject.Inject;
+import org.apache.ignite.IgnitionManager;
+import org.apache.ignite.internal.cli.core.repl.ConnectionHeartBeat;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.core.repl.prompt.ReplPromptProvider;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Spy;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Spy
+ @Inject
+ private ReplPromptProvider replPromptProvider;
+
+ @BeforeEach
+ void setUp() {
+ //Set connection check timeout to 1 sec to make test fast
+ ConnectionHeartBeat.CLI_CHECK_CONNECTION_PERIOD_SECONDS = 1L;
+ }
+
+ @AfterEach
+ void teardown() {
+ // Start all nodes
+ allNodeNames().forEach(this::startNode);
Review Comment:
I don't think you want to start all nodes on the test tear-down.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.cli.commands;
+
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.after;
+import static org.mockito.Mockito.verify;
+
+import jakarta.inject.Inject;
+import org.apache.ignite.IgnitionManager;
+import org.apache.ignite.internal.cli.core.repl.ConnectionHeartBeat;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.core.repl.prompt.ReplPromptProvider;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Spy;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Spy
+ @Inject
+ private ReplPromptProvider replPromptProvider;
+
+ @BeforeEach
+ void setUp() {
+ //Set connection check timeout to 1 sec to make test fast
+ ConnectionHeartBeat.CLI_CHECK_CONNECTION_PERIOD_SECONDS = 1L;
Review Comment:
We tend not to use global static mutable variables (only in extreme cases). I would ask you to put the configuration value into `ConnectionHeartBeat` via a constructor. In test, you can create the instance directly or using Micronaut's configuration property.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1287214601
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/AsyncConnectionEventListener.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.cli.event;
+
+import org.apache.ignite.internal.cli.core.repl.ConnectEvent;
+import org.apache.ignite.internal.cli.core.repl.SessionInfo;
+
+/**
+ * Listener of any connection related events.
+ */
+public abstract class AsyncConnectionEventListener implements EventListener {
Review Comment:
Why does in have `Async` prefix? I don't see any asynchronous logic 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1285634331
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
+ this.startNode(nodeName);
Review Comment:
Could you mention this in the comment?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] nevdmitry commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "nevdmitry (via GitHub)" <gi...@apache.org>.
nevdmitry commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1284705305
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
Review Comment:
it is doesn't work with @Value
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1286811430
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventSubscriptionManager;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name = "cli.check.connection.period.second", value = "1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventSubscriptionManager eventSubscriptionManager;
+
+ @Value("${cli.check.connection.period.second}")
+ private long cliCheckConnectionPeriodSecond;
+
+ private final AtomicInteger connectionLostCount = new AtomicInteger(0);
+ private final AtomicInteger connectionRestoredCount = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ connectionLostCount.set(0);
+ connectionRestoredCount.set(0);
+ EventListener connectionRestoredEventListener = (event) -> {
+ connectionRestoredCount.incrementAndGet();
+ };
+
+ EventListener connectionLostEventListener = (event) -> {
+ connectionLostCount.incrementAndGet();
+ };
+
+ // Register listeners
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_LOST, connectionLostEventListener);
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_RESTORED, connectionRestoredEventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ assertEquals(0, connectionLostCount.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
Review Comment:
```suggestion
stopNode(nodeName);
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventSubscriptionManager;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name = "cli.check.connection.period.second", value = "1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventSubscriptionManager eventSubscriptionManager;
+
+ @Value("${cli.check.connection.period.second}")
+ private long cliCheckConnectionPeriodSecond;
+
+ private final AtomicInteger connectionLostCount = new AtomicInteger(0);
+ private final AtomicInteger connectionRestoredCount = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ connectionLostCount.set(0);
+ connectionRestoredCount.set(0);
+ EventListener connectionRestoredEventListener = (event) -> {
+ connectionRestoredCount.incrementAndGet();
+ };
+
+ EventListener connectionLostEventListener = (event) -> {
+ connectionLostCount.incrementAndGet();
+ };
+
+ // Register listeners
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_LOST, connectionLostEventListener);
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_RESTORED, connectionRestoredEventListener);
Review Comment:
I'd remove the local variables, or at least remove unnecessary braces and parentheses
```suggestion
// Register listeners
eventSubscriptionManager.subscribe(EventType.CONNECTION_LOST, event -> connectionLostCount.incrementAndGet());
eventSubscriptionManager.subscribe(EventType.CONNECTION_RESTORED, event -> connectionRestoredCount.incrementAndGet());
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventSubscriptionManager;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name = "cli.check.connection.period.second", value = "1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventSubscriptionManager eventSubscriptionManager;
+
+ @Value("${cli.check.connection.period.second}")
+ private long cliCheckConnectionPeriodSecond;
+
+ private final AtomicInteger connectionLostCount = new AtomicInteger(0);
+ private final AtomicInteger connectionRestoredCount = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ connectionLostCount.set(0);
+ connectionRestoredCount.set(0);
+ EventListener connectionRestoredEventListener = (event) -> {
+ connectionRestoredCount.incrementAndGet();
+ };
+
+ EventListener connectionLostEventListener = (event) -> {
+ connectionLostCount.incrementAndGet();
+ };
+
+ // Register listeners
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_LOST, connectionLostEventListener);
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_RESTORED, connectionRestoredEventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ assertEquals(0, connectionLostCount.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionLostCount.get() == 1);
+
+ // Tear down. Restore initial state of node to exclude any impact on next test.
+ this.startNode(nodeName);
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void restoreConnectionAfterConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
Review Comment:
```suggestion
stopNode(nodeName);
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventSubscriptionManager;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name = "cli.check.connection.period.second", value = "1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventSubscriptionManager eventSubscriptionManager;
+
+ @Value("${cli.check.connection.period.second}")
+ private long cliCheckConnectionPeriodSecond;
+
+ private final AtomicInteger connectionLostCount = new AtomicInteger(0);
+ private final AtomicInteger connectionRestoredCount = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ connectionLostCount.set(0);
+ connectionRestoredCount.set(0);
+ EventListener connectionRestoredEventListener = (event) -> {
+ connectionRestoredCount.incrementAndGet();
+ };
+
+ EventListener connectionLostEventListener = (event) -> {
+ connectionLostCount.incrementAndGet();
+ };
+
+ // Register listeners
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_LOST, connectionLostEventListener);
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_RESTORED, connectionRestoredEventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ assertEquals(0, connectionLostCount.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionLostCount.get() == 1);
+
+ // Tear down. Restore initial state of node to exclude any impact on next test.
+ this.startNode(nodeName);
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void restoreConnectionAfterConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ // Then connection lost event obtained
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionLostCount.get() == 1);
+
+ // When
+ this.startNode(nodeName);
Review Comment:
```suggestion
startNode(nodeName);
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventSubscriptionManager;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name = "cli.check.connection.period.second", value = "1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventSubscriptionManager eventSubscriptionManager;
+
+ @Value("${cli.check.connection.period.second}")
+ private long cliCheckConnectionPeriodSecond;
+
+ private final AtomicInteger connectionLostCount = new AtomicInteger(0);
+ private final AtomicInteger connectionRestoredCount = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ connectionLostCount.set(0);
+ connectionRestoredCount.set(0);
+ EventListener connectionRestoredEventListener = (event) -> {
+ connectionRestoredCount.incrementAndGet();
+ };
+
+ EventListener connectionLostEventListener = (event) -> {
+ connectionLostCount.incrementAndGet();
+ };
+
+ // Register listeners
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_LOST, connectionLostEventListener);
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_RESTORED, connectionRestoredEventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ assertEquals(0, connectionLostCount.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionLostCount.get() == 1);
Review Comment:
I'd combine the checks:
```suggestion
await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(
() -> connectionRestoredCount.get() == 1 && connectionLostCount.get() == 1
);
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * 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.cli.core.repl;
+
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+import javax.annotation.Nullable;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventPublisher;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.apache.ignite.internal.cli.event.Events;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat implements EventListener {
+
+ private static final IgniteLogger log = CliLoggers.forClass(ConnectionHeartBeat.class);
Review Comment:
Statics should be in uppercase
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/EventFactory.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.cli.event;
+
+import jakarta.inject.Singleton;
+import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+
+/**
+ * Register listeners and produces events.
+ */
+@Singleton
+public class EventFactory implements EventPublisher, EventSubscriptionManager {
+
+ private static final IgniteLogger log = CliLoggers.forClass(EventFactory.class);
+
+ /** All listeners. */
+ private final ConcurrentHashMap<EventType, List<EventListener>> listeners = new ConcurrentHashMap<>();
+
+ public EventFactory() {
+ }
+
Review Comment:
```suggestion
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventSubscriptionManager;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name = "cli.check.connection.period.second", value = "1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventSubscriptionManager eventSubscriptionManager;
+
+ @Value("${cli.check.connection.period.second}")
+ private long cliCheckConnectionPeriodSecond;
+
+ private final AtomicInteger connectionLostCount = new AtomicInteger(0);
+ private final AtomicInteger connectionRestoredCount = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ connectionLostCount.set(0);
+ connectionRestoredCount.set(0);
+ EventListener connectionRestoredEventListener = (event) -> {
+ connectionRestoredCount.incrementAndGet();
+ };
+
+ EventListener connectionLostEventListener = (event) -> {
+ connectionLostCount.incrementAndGet();
+ };
+
+ // Register listeners
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_LOST, connectionLostEventListener);
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_RESTORED, connectionRestoredEventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ assertEquals(0, connectionLostCount.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionLostCount.get() == 1);
+
+ // Tear down. Restore initial state of node to exclude any impact on next test.
+ this.startNode(nodeName);
Review Comment:
```suggestion
startNode(nodeName);
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/EventFactory.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.cli.event;
+
+import jakarta.inject.Singleton;
+import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+
+/**
+ * Register listeners and produces events.
+ */
+@Singleton
+public class EventFactory implements EventPublisher, EventSubscriptionManager {
+
+ private static final IgniteLogger log = CliLoggers.forClass(EventFactory.class);
Review Comment:
LOG
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventSubscriptionManager;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name = "cli.check.connection.period.second", value = "1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventSubscriptionManager eventSubscriptionManager;
+
+ @Value("${cli.check.connection.period.second}")
+ private long cliCheckConnectionPeriodSecond;
+
+ private final AtomicInteger connectionLostCount = new AtomicInteger(0);
+ private final AtomicInteger connectionRestoredCount = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ connectionLostCount.set(0);
+ connectionRestoredCount.set(0);
+ EventListener connectionRestoredEventListener = (event) -> {
+ connectionRestoredCount.incrementAndGet();
+ };
+
+ EventListener connectionLostEventListener = (event) -> {
+ connectionLostCount.incrementAndGet();
+ };
+
+ // Register listeners
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_LOST, connectionLostEventListener);
+ eventSubscriptionManager.subscribe(EventType.CONNECTION_RESTORED, connectionRestoredEventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ assertEquals(0, connectionLostCount.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ // Listener was invoked
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionLostCount.get() == 1);
+
+ // Tear down. Restore initial state of node to exclude any impact on next test.
+ this.startNode(nodeName);
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void restoreConnectionAfterConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ // Then connection lost event obtained
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionRestoredCount.get() == 1);
+ await().timeout(cliCheckConnectionPeriodSecond * 2, TimeUnit.SECONDS).until(() -> connectionLostCount.get() == 1);
Review Comment:
And here and below as well.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/registry/impl/ClusterConfigRegistryImpl.java:
##########
@@ -24,26 +24,35 @@
import org.apache.ignite.internal.cli.call.configuration.ClusterConfigShowCallInput;
import org.apache.ignite.internal.cli.call.configuration.JsonString;
import org.apache.ignite.internal.cli.core.call.DefaultCallOutput;
-import org.apache.ignite.internal.cli.core.repl.AsyncSessionEventListener;
+import org.apache.ignite.internal.cli.core.repl.ConnectEvent;
import org.apache.ignite.internal.cli.core.repl.SessionInfo;
import org.apache.ignite.internal.cli.core.repl.registry.ClusterConfigRegistry;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
import org.jetbrains.annotations.Nullable;
/** Implementation of {@link ClusterConfigRegistry}. */
@Singleton
-public class ClusterConfigRegistryImpl implements ClusterConfigRegistry, AsyncSessionEventListener {
+public class ClusterConfigRegistryImpl implements ClusterConfigRegistry, EventListener {
private final ClusterConfigShowCall clusterConfigShowCall;
+ @Nullable
private LazyObjectRef<Config> configRef;
public ClusterConfigRegistryImpl(ClusterConfigShowCall clusterConfigShowCall) {
this.clusterConfigShowCall = clusterConfigShowCall;
}
@Override
- public void onConnect(SessionInfo sessionInfo) {
- configRef = new LazyObjectRef<>(() -> fetchConfig(sessionInfo));
+ public void onEvent(Event event) {
+ if (EventType.CONNECT == event.eventType()) {
+ ConnectEvent connectEvent = (ConnectEvent) event;
+ configRef = new LazyObjectRef<>(() -> fetchConfig(connectEvent.sessionInfo()));
+ } else if (EventType.DISCONNECT == event.eventType()) {
+ configRef = null;
+ }
}
Review Comment:
Seeing this pattern many times I wonder whether it's worth changing `onEvent` to be default method and adding `onConnect` and `onDisconnect` default methods to the `EventListener`?
This will keep the current code intact.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1284461787
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
Review Comment:
We tend to link any todo with the ticket. Could you create one or fix todo here?
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
Review Comment:
```suggestion
private final long CLI_CHECK_CONNECTION_PERIOD_SECONDS = 1;
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.cli.core.repl;
+
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat implements EventListener {
+
+ private static final IgniteLogger log = CliLoggers.forClass(ConnectionHeartBeat.class);
+
+ /** CLI check connection period period. */
+ private final long cliCheckConnectionPeriodSecond;
+
+ /** Scheduled executor for connection heartbeat. */
+ @Nullable
+ private ScheduledExecutorService scheduledConnectionHeartbeatExecutor;
+
+ private final ApiClientFactory clientFactory;
+
+ private final EventFactory eventFactory;
+
+ private final AtomicBoolean connected = new AtomicBoolean(false);
+
+ /**
+ * Created instance of connection heartbeat.
Review Comment:
```suggestion
* Creates the instance of connection heartbeat.
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.cli.core.repl;
+
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat implements EventListener {
+
+ private static final IgniteLogger log = CliLoggers.forClass(ConnectionHeartBeat.class);
+
+ /** CLI check connection period period. */
+ private final long cliCheckConnectionPeriodSecond;
+
+ /** Scheduled executor for connection heartbeat. */
+ @Nullable
+ private ScheduledExecutorService scheduledConnectionHeartbeatExecutor;
+
+ private final ApiClientFactory clientFactory;
+
+ private final EventFactory eventFactory;
+
+ private final AtomicBoolean connected = new AtomicBoolean(false);
+
+ /**
+ * Created instance of connection heartbeat.
+ *
+ * @param clientFactory api client factory.
+ * @param eventFactory event factory.
+ */
+ public ConnectionHeartBeat(@Value("${cli.check.connection.period.second:5}") long cliCheckConnectionPeriodSecond,
+ ApiClientFactory clientFactory,
+ EventFactory eventFactory) {
+ this.clientFactory = clientFactory;
+ this.eventFactory = eventFactory;
+ this.cliCheckConnectionPeriodSecond = cliCheckConnectionPeriodSecond;
+ }
+
+ /**
+ * Starts connection heartbeat. By default connection will be checked every 5 sec.
+ *
+ * @param sessionInfo session info with node url
+ */
+ private void onConnect(SessionInfo sessionInfo) {
+ //eventFactory.fireEvent(EventType.CONNECTION_RESTORED, new ConnectionStatusEvent());
+
+ if (scheduledConnectionHeartbeatExecutor == null) {
+ scheduledConnectionHeartbeatExecutor =
+ Executors.newScheduledThreadPool(1, new NamedThreadFactory("cli-check-connection-thread", log));
+
+ //Start connection heart beat
+ scheduledConnectionHeartbeatExecutor.scheduleAtFixedRate(
+ () -> pingConnection(sessionInfo.nodeUrl()),
+ 0,
+ cliCheckConnectionPeriodSecond,
+ TimeUnit.SECONDS
+ );
+ }
+ }
+
+ /**
+ * Stops connection heartbeat.
+ */
+ private void onDisconnect() {
+ if (scheduledConnectionHeartbeatExecutor != null) {
+ scheduledConnectionHeartbeatExecutor.shutdownNow();
+ scheduledConnectionHeartbeatExecutor = null;
+ }
+ }
+
+ private void pingConnection(String nodeUrl) {
+ try {
+ new NodeManagementApi(clientFactory.getClient(nodeUrl)).nodeState();
+ if (!connected.get()) {
+ connected.compareAndSet(false, true);
Review Comment:
That is not thread-safe. I would suggest calling `connected.compareAndSet(false, true)` and check the result of the operation.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
+ this.startNode(nodeName);
Review Comment:
As far as I see it is already stopped.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
+ this.startNode(nodeName);
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void restoreConnectionAfterConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Then
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ // When
+ this.startNode(nodeName);
+
+ //Then
Review Comment:
```suggestion
// Then
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
Review Comment:
It is better not to use if statement but register two separate event listeners
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
Review Comment:
```suggestion
// Tear down
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
+ this.startNode(nodeName);
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void restoreConnectionAfterConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Then
Review Comment:
```suggestion
// Then
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
+ this.startNode(nodeName);
Review Comment:
I think stop should be invoked instead.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
Review Comment:
```suggestion
private final AtomicInteger connectionLostCount = new AtomicInteger(0);
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
Review Comment:
```suggestion
private final AtomicInteger connectionRestoredCount = new AtomicInteger(0);
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
Review Comment:
```suggestion
// Listener was invoked
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/connect/ConnectCall.java:
##########
@@ -56,15 +61,18 @@ public class ConnectCall implements Call<UrlCallInput, String> {
private final JdbcUrlFactory jdbcUrlFactory;
+ private final EventFactory eventFactory;
+
/**
* Constructor.
*/
public ConnectCall(Session session, StateConfigProvider stateConfigProvider, ApiClientFactory clientFactory,
- JdbcUrlFactory jdbcUrlFactory) {
+ JdbcUrlFactory jdbcUrlFactory, ConnectionHeartBeat connectionHeartBeat, EventFactory eventFactory) {
Review Comment:
```suggestion
JdbcUrlFactory jdbcUrlFactory, EventFactory eventFactory) {
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
+ this.startNode(nodeName);
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void restoreConnectionAfterConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Then
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ // When
+ this.startNode(nodeName);
+
+ //Then
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 2);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
Review Comment:
We've already waited for this condition.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/connect/DisconnectCall.java:
##########
@@ -17,35 +17,40 @@
package org.apache.ignite.internal.cli.call.connect;
-import jakarta.inject.Inject;
import jakarta.inject.Singleton;
import org.apache.ignite.internal.cli.core.call.Call;
import org.apache.ignite.internal.cli.core.call.CallOutput;
import org.apache.ignite.internal.cli.core.call.DefaultCallOutput;
import org.apache.ignite.internal.cli.core.call.EmptyCallInput;
+import org.apache.ignite.internal.cli.core.repl.ConnectionHeartBeat;
import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.core.repl.SessionDisconnectEvent;
import org.apache.ignite.internal.cli.core.repl.SessionInfo;
import org.apache.ignite.internal.cli.core.style.component.MessageUiComponent;
import org.apache.ignite.internal.cli.core.style.element.UiElements;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventType;
/**
* Call for disconnect.
*/
@Singleton
public class DisconnectCall implements Call<EmptyCallInput, String> {
- @Inject
private final Session session;
- public DisconnectCall(Session session) {
+ private final EventFactory eventFactory;
+
+ public DisconnectCall(Session session, ConnectionHeartBeat connectionHeartBeat, EventFactory eventFactory) {
Review Comment:
```suggestion
public DisconnectCall(Session session, EventFactory eventFactory) {
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.cli.core.repl;
+
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat implements EventListener {
+
+ private static final IgniteLogger log = CliLoggers.forClass(ConnectionHeartBeat.class);
+
+ /** CLI check connection period period. */
+ private final long cliCheckConnectionPeriodSecond;
+
+ /** Scheduled executor for connection heartbeat. */
+ @Nullable
+ private ScheduledExecutorService scheduledConnectionHeartbeatExecutor;
+
+ private final ApiClientFactory clientFactory;
+
+ private final EventFactory eventFactory;
+
+ private final AtomicBoolean connected = new AtomicBoolean(false);
+
+ /**
+ * Created instance of connection heartbeat.
+ *
+ * @param clientFactory api client factory.
+ * @param eventFactory event factory.
+ */
+ public ConnectionHeartBeat(@Value("${cli.check.connection.period.second:5}") long cliCheckConnectionPeriodSecond,
+ ApiClientFactory clientFactory,
+ EventFactory eventFactory) {
+ this.clientFactory = clientFactory;
+ this.eventFactory = eventFactory;
+ this.cliCheckConnectionPeriodSecond = cliCheckConnectionPeriodSecond;
+ }
+
+ /**
+ * Starts connection heartbeat. By default connection will be checked every 5 sec.
+ *
+ * @param sessionInfo session info with node url
+ */
+ private void onConnect(SessionInfo sessionInfo) {
+ //eventFactory.fireEvent(EventType.CONNECTION_RESTORED, new ConnectionStatusEvent());
Review Comment:
commented code
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.cli.core.repl;
+
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat implements EventListener {
+
+ private static final IgniteLogger log = CliLoggers.forClass(ConnectionHeartBeat.class);
+
+ /** CLI check connection period period. */
+ private final long cliCheckConnectionPeriodSecond;
+
+ /** Scheduled executor for connection heartbeat. */
+ @Nullable
+ private ScheduledExecutorService scheduledConnectionHeartbeatExecutor;
+
+ private final ApiClientFactory clientFactory;
+
+ private final EventFactory eventFactory;
+
+ private final AtomicBoolean connected = new AtomicBoolean(false);
+
+ /**
+ * Created instance of connection heartbeat.
+ *
+ * @param clientFactory api client factory.
+ * @param eventFactory event factory.
+ */
+ public ConnectionHeartBeat(@Value("${cli.check.connection.period.second:5}") long cliCheckConnectionPeriodSecond,
+ ApiClientFactory clientFactory,
+ EventFactory eventFactory) {
+ this.clientFactory = clientFactory;
+ this.eventFactory = eventFactory;
+ this.cliCheckConnectionPeriodSecond = cliCheckConnectionPeriodSecond;
+ }
+
+ /**
+ * Starts connection heartbeat. By default connection will be checked every 5 sec.
+ *
+ * @param sessionInfo session info with node url
+ */
+ private void onConnect(SessionInfo sessionInfo) {
+ //eventFactory.fireEvent(EventType.CONNECTION_RESTORED, new ConnectionStatusEvent());
+
+ if (scheduledConnectionHeartbeatExecutor == null) {
+ scheduledConnectionHeartbeatExecutor =
+ Executors.newScheduledThreadPool(1, new NamedThreadFactory("cli-check-connection-thread", log));
+
+ //Start connection heart beat
Review Comment:
```suggestion
// Start connection heart beat.
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/Event.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * 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.cli.event;
+
+/**
+ * The event cas which is produced by event producer component.
+ */
+public interface Event {
+}
Review Comment:
missed eof
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ReplBuilder.java:
##########
@@ -152,4 +155,15 @@ public ReplBuilder withAutosuggestionsWidgets() {
this.autosuggestionsWidgetsEnabled = true;
return this;
}
+
+ /**
+ * Builder setter of {@code aliases} field.
+ *
+ * @param eventSubscriber ???? map of aliases for commands.
Review Comment:
```suggestion
* @param eventSubscriber map of aliases for commands.
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/PeriodicSessionTaskExecutor.java:
##########
@@ -24,35 +24,48 @@
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
import org.apache.ignite.internal.cli.logger.CliLoggers;
import org.apache.ignite.internal.logger.IgniteLogger;
import org.apache.ignite.internal.thread.NamedThreadFactory;
import org.jetbrains.annotations.Nullable;
/** Executes tasks periodically while the session is connected. */
@Singleton
-public class PeriodicSessionTaskExecutor implements AsyncSessionEventListener {
+public class PeriodicSessionTaskExecutor implements EventListener {
private static final IgniteLogger LOG = CliLoggers.forClass(PeriodicSessionTaskExecutor.class);
@Nullable
private ScheduledExecutorService executor;
private final List<? extends PeriodicSessionTask> tasks;
+ //private final EventFactory eventFactory;
Review Comment:
commented
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/EventFactory.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.cli.event;
+
+import jakarta.inject.Singleton;
+import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+
+/**
+ * Register listeners and produces events.
+ */
+@Singleton
+public class EventFactory {
+
+ private static final IgniteLogger log = CliLoggers.forClass(EventFactory.class);
+
+ /** All listeners. */
+ private final ConcurrentHashMap<EventType, List<EventListener>> listeners = new ConcurrentHashMap<>();
+
+ public EventFactory() {
+ }
+
+ /**
+ * Registers an event listener.
+ *
+ * @param eventType type of event to listen.
+ * @param eventListener event listener.
+ */
+ public void listen(EventType eventType, EventListener eventListener) {
+ listeners.computeIfAbsent(eventType, evtKey -> new CopyOnWriteArrayList<>()).add(eventListener);
+ }
+
+ /**
+ * Removes a listener associated with the event type.
+ *
+ * @param eventType type of event to listen.
+ * @param eventListener event listener.
+ */
+ public void removeListener(EventType eventType, EventListener eventListener) {
+ listeners.computeIfAbsent(eventType, eventT -> new CopyOnWriteArrayList<>()).remove(eventListener);
Review Comment:
Use`computeIfPresent` here
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ReplBuilder.java:
##########
@@ -152,4 +155,15 @@ public ReplBuilder withAutosuggestionsWidgets() {
this.autosuggestionsWidgetsEnabled = true;
return this;
}
+
+ /**
+ * Builder setter of {@code aliases} field.
+ *
+ * @param eventSubscriber ???? map of aliases for commands.
Review Comment:
the whole java doc here looks like it came from another file.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] nevdmitry commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "nevdmitry (via GitHub)" <gi...@apache.org>.
nevdmitry commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1284708406
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
+ connectionLost.set(0);
+ connectionRestored.set(0);
+ EventListener eventListener = (eventType, event) -> {
+ if (EventType.CONNECTION_LOST == eventType) {
+ connectionLost.incrementAndGet();
+ } else if (EventType.CONNECTION_RESTORED == eventType) {
+ connectionRestored.incrementAndGet();
+ }
+ };
+
+ //Register listeners
+ eventFactory.listen(EventType.CONNECTION_LOST, eventListener);
+ eventFactory.listen(EventType.CONNECTION_RESTORED, eventListener);
+ }
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_RESTORED on connection start")
+ void connectionEstablished() {
+ // Given null session info before connect
+ assertNull(session.info());
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ assertEquals(0, connectionLost.get());
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void onConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Listener was invoked
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ //Tear down
+ this.startNode(nodeName);
+ }
+
+ @Test
+ @DisplayName("Should send event CONNECTION_LOST on cluster stop")
+ void restoreConnectionAfterConnectionLost() {
+ // Given connected cli
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // When stop node
+ String nodeName = session.info().nodeName();
+ this.stopNode(nodeName);
+
+ //Then
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 1);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
+
+ // When
+ this.startNode(nodeName);
+
+ //Then
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionRestored.get() == 2);
+ await().timeout(CLI_CHECK_CONNECTION_PERIOD_SECONDS * 2, TimeUnit.SECONDS).until(() -> connectionLost.get() == 1);
Review Comment:
I check connection restore event here after node 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1275190008
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionStatusTest.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import jakarta.inject.Inject;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.core.repl.SessionInfo.ConnectionStatus;
+import org.apache.ignite.internal.cli.core.repl.prompt.PromptProvider;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import picocli.CommandLine.Help.Ansi;
+
+class ItConnectionStatusTest extends CliCommandTestInitializedIntegrationBase {
+ @Inject
+ PromptProvider promptProvider;
+
+ @Inject
+ Session session;
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return TopLevelCliReplCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should connect to cluster with default url")
+ void connectWithDefaultUrl() throws NoSuchFieldException, IllegalAccessException {
+ // Given prompt before connect
+ String promptBefore = Ansi.OFF.string(promptProvider.getPrompt());
+ assertThat(promptBefore).isEqualTo("[disconnected]> ");
+
+ // When connect without parameters
+ execute("connect");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to http://localhost:10300")
+ );
+
+ // And prompt is changed to green (connection status OPEN)
+ assertEquals(ConnectionStatus.OPEN, session.info().connectionStatus());
Review Comment:
If you check `ConnectionStatus` after an action it would be nice to check `ConnectionStatus` before, not the prompt.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.cli.core.repl;
+
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.internal.cli.core.repl.SessionInfo.ConnectionStatus;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat {
+
+ private static final IgniteLogger log = CliLoggers.forClass(Session.class);
Review Comment:
```suggestion
private static final IgniteLogger log = CliLoggers.forClass(ConnectionHeartBeat.class);
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/core/repl/executor/ItIgnitePicocliCommandsTest.java:
##########
@@ -245,7 +246,7 @@ void nodeConfigShowSuggested(ParsedLine givenParsedLine) {
}
private void connected() {
- session.connect(new SessionInfo(DEFAULT_REST_URL, null, null, null));
+ session.connect(new SessionInfo(DEFAULT_REST_URL, null, null, null, ConnectionStatus.OPEN));
Review Comment:
That is getting weird. Could you please get rid of a constructor in SessionInfo and create a builder?
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.cli.core.repl;
+
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.internal.cli.core.repl.SessionInfo.ConnectionStatus;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat {
+
+ private static final IgniteLogger log = CliLoggers.forClass(Session.class);
+
+ /** CLI check connection period period. */
+ private static final int CLI_CHECK_CONNECTION_PERIOD_SECONDS = 10;
+
+ /** Scheduled executor for idle safe time sync. */
+ private final ScheduledExecutorService scheduledIdleSafeTimeSyncExecutor =
+ Executors.newScheduledThreadPool(1, new NamedThreadFactory("cli-check-connection-thread", log));
+
+ /**
+ * Start connection heartbeat.
+ *
+ * @param session session
+ * @param clientFactory api client factory to ping connection.
+ */
+ public void start(Session session, ApiClientFactory clientFactory) {
+ scheduledIdleSafeTimeSyncExecutor.scheduleAtFixedRate(
+ () -> pingConnection(session, clientFactory),
+ 0,
+ CLI_CHECK_CONNECTION_PERIOD_SECONDS,
+ TimeUnit.SECONDS
+ );
+ }
+
+ public void stop() {
+ scheduledIdleSafeTimeSyncExecutor.shutdownNow();
+ }
+
+ private static void pingConnection(Session session, ApiClientFactory clientFactory) {
+ try {
+ new NodeManagementApi(clientFactory.getClient(session.info().nodeUrl())).nodeState();
+ SessionInfo currentSessionInfo = session.info();
+ session.updateSessionInfo(new SessionInfo(
Review Comment:
That is not a thread-safe solution. You can get `session.info()` and get it updated by disconnect/connect or some other things. I mean if you read some state, do action, and update state – this should be thread safe.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.cli.core.repl;
+
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.internal.cli.core.repl.SessionInfo.ConnectionStatus;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat {
+
+ private static final IgniteLogger log = CliLoggers.forClass(Session.class);
+
+ /** CLI check connection period period. */
+ private static final int CLI_CHECK_CONNECTION_PERIOD_SECONDS = 10;
+
+ /** Scheduled executor for idle safe time sync. */
+ private final ScheduledExecutorService scheduledIdleSafeTimeSyncExecutor =
+ Executors.newScheduledThreadPool(1, new NamedThreadFactory("cli-check-connection-thread", log));
+
+ /**
+ * Start connection heartbeat.
+ *
+ * @param session session
+ * @param clientFactory api client factory to ping connection.
+ */
+ public void start(Session session, ApiClientFactory clientFactory) {
+ scheduledIdleSafeTimeSyncExecutor.scheduleAtFixedRate(
+ () -> pingConnection(session, clientFactory),
+ 0,
+ CLI_CHECK_CONNECTION_PERIOD_SECONDS,
+ TimeUnit.SECONDS
+ );
+ }
+
+ public void stop() {
+ scheduledIdleSafeTimeSyncExecutor.shutdownNow();
+ }
+
+ private static void pingConnection(Session session, ApiClientFactory clientFactory) {
+ try {
+ new NodeManagementApi(clientFactory.getClient(session.info().nodeUrl())).nodeState();
+ SessionInfo currentSessionInfo = session.info();
+ session.updateSessionInfo(new SessionInfo(
Review Comment:
That is not a thread-safe solution. You can get `session.info()` and get it updated by disconnect/connect or some other things. I mean if you read some state, do action, and update state – this should be thread safe.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.cli.core.repl;
+
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.internal.cli.core.repl.SessionInfo.ConnectionStatus;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat {
+
+ private static final IgniteLogger log = CliLoggers.forClass(Session.class);
+
+ /** CLI check connection period period. */
+ private static final int CLI_CHECK_CONNECTION_PERIOD_SECONDS = 10;
Review Comment:
10 seconds is too long a time period. The peak time user might see the wrong state is (period + period-1) = 19 sec. I suggest having at lead 5 sec.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/connect/ConnectCall.java:
##########
@@ -81,8 +86,8 @@ public CallOutput<String> execute(UrlCallInput input) {
stateConfigProvider.get().setProperty(CliConfigKeys.LAST_CONNECTED_URL.value(), nodeUrl);
String jdbcUrl = jdbcUrlFactory.constructJdbcUrl(configuration, nodeUrl);
- session.connect(new SessionInfo(nodeUrl, fetchNodeName(nodeUrl), jdbcUrl, username));
-
+ session.connect(new SessionInfo(nodeUrl, fetchNodeName(nodeUrl), jdbcUrl, username, ConnectionStatus.OPEN));
+ connectionHeartBeat.start(session, clientFactory);
Review Comment:
We have `AsyncSessionEventListener`, I think you can use it for connect/disconnect 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] nevdmitry commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "nevdmitry (via GitHub)" <gi...@apache.org>.
nevdmitry commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1287031241
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/EventFactory.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.cli.event;
+
+import jakarta.inject.Singleton;
+import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+
+/**
+ * Register listeners and produces events.
+ */
+@Singleton
+public class EventFactory implements EventPublisher, EventSubscriptionManager {
+
+ private static final IgniteLogger log = CliLoggers.forClass(EventFactory.class);
Review Comment:
looks like we need to update checkstyle rules
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] nevdmitry commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "nevdmitry (via GitHub)" <gi...@apache.org>.
nevdmitry commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1287072053
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/registry/impl/ClusterConfigRegistryImpl.java:
##########
@@ -24,26 +24,35 @@
import org.apache.ignite.internal.cli.call.configuration.ClusterConfigShowCallInput;
import org.apache.ignite.internal.cli.call.configuration.JsonString;
import org.apache.ignite.internal.cli.core.call.DefaultCallOutput;
-import org.apache.ignite.internal.cli.core.repl.AsyncSessionEventListener;
+import org.apache.ignite.internal.cli.core.repl.ConnectEvent;
import org.apache.ignite.internal.cli.core.repl.SessionInfo;
import org.apache.ignite.internal.cli.core.repl.registry.ClusterConfigRegistry;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
import org.jetbrains.annotations.Nullable;
/** Implementation of {@link ClusterConfigRegistry}. */
@Singleton
-public class ClusterConfigRegistryImpl implements ClusterConfigRegistry, AsyncSessionEventListener {
+public class ClusterConfigRegistryImpl implements ClusterConfigRegistry, EventListener {
private final ClusterConfigShowCall clusterConfigShowCall;
+ @Nullable
private LazyObjectRef<Config> configRef;
public ClusterConfigRegistryImpl(ClusterConfigShowCall clusterConfigShowCall) {
this.clusterConfigShowCall = clusterConfigShowCall;
}
@Override
- public void onConnect(SessionInfo sessionInfo) {
- configRef = new LazyObjectRef<>(() -> fetchConfig(sessionInfo));
+ public void onEvent(Event event) {
+ if (EventType.CONNECT == event.eventType()) {
+ ConnectEvent connectEvent = (ConnectEvent) event;
+ configRef = new LazyObjectRef<>(() -> fetchConfig(connectEvent.sessionInfo()));
+ } else if (EventType.DISCONNECT == event.eventType()) {
+ configRef = null;
+ }
}
Review Comment:
Now we have 4 event types, onConnect and onDisconnect only 2 of them but I agree that this pattern uses in 5 places, will introduce ConnectionEventListener
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1285688630
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/EventListeningActivationPoint.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.cli.core.repl;
+
+import jakarta.inject.Inject;
+import jakarta.inject.Singleton;
+import org.apache.ignite.internal.cli.core.repl.prompt.ReplPromptProvider;
+import org.apache.ignite.internal.cli.core.repl.registry.impl.ClusterConfigRegistryImpl;
+import org.apache.ignite.internal.cli.core.repl.registry.impl.MetricRegistryImpl;
+import org.apache.ignite.internal.cli.core.repl.registry.impl.NodeConfigRegistryImpl;
+import org.apache.ignite.internal.cli.core.repl.registry.impl.UnitsRegistryImpl;
+import org.apache.ignite.internal.cli.event.EventSubscriber;
+import org.apache.ignite.internal.cli.event.EventType;
+
+/**
+ * Subscribes beans on appropriate events.
+ */
+@Singleton
+public class EventListeningActivationPoint {
+
+ @Inject
+ private EventSubscriber eventSubscriber;
Review Comment:
Can you please rename `EventSubscriber` to something like `EventSubscribtionManager`? Because I'm confused a little. When I read "Event Subscriber" I think of it like this object is subscribed to event but not subscribes others.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/CliCommandTestNotInitializedIntegrationBase.java:
##########
@@ -89,13 +94,14 @@ public void setUp(TestInfo testInfo) throws Exception {
cmd = new CommandLine(getCommandClass(), new MicronautFactory(context))
.registerConverter(NodeNameOrUrl.class, new NodeNameOrUrlConverter(nodeNameRegistry));
cmd.setDefaultValueProvider(configDefaultValueProvider);
+ eventListeningActivationPoint.subscribe();
resetOutput();
CommandLineContextProvider.setCmd(cmd);
}
@AfterEach
public void tearDown() {
- session.disconnect();
+ eventPublisher.fireEvent(Events.disconnect());
Review Comment:
`Publisher` is supposed to `publish`. Could you rename the method?
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/Event.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * 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.cli.event;
+
+/**
+ * The event cas which is produced by event producer component.
+ */
+public interface Event {
+ EventType eventType();
+}
Review Comment:
Missed eof. It is better to configure your IDEA to do it automatically.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/ConnectionHeartBeat.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.cli.core.repl;
+
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Singleton;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.ignite.internal.cli.core.rest.ApiClientFactory;
+import org.apache.ignite.internal.cli.event.Event;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventPublisher;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.apache.ignite.internal.cli.event.Events;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.thread.NamedThreadFactory;
+import org.apache.ignite.rest.client.api.NodeManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Connection to node heart beat.
+ */
+@Singleton
+public class ConnectionHeartBeat implements EventListener {
+
+ private static final IgniteLogger log = CliLoggers.forClass(ConnectionHeartBeat.class);
+
+ /** CLI check connection period period. */
+ private final long cliCheckConnectionPeriodSecond;
+
+ /** Scheduled executor for connection heartbeat. */
+ @Nullable
+ private ScheduledExecutorService scheduledConnectionHeartbeatExecutor;
+
+ private final ApiClientFactory clientFactory;
+
+ private final EventPublisher eventPublisher;
+
+ private final AtomicBoolean connected = new AtomicBoolean(false);
+
+ /**
+ * Creates the instance of connection heartbeat.
+ *
+ * @param clientFactory api client factory.
+ * @param eventPublisher event publisher.
+ */
+ public ConnectionHeartBeat(@Value("${cli.check.connection.period.second:5}") long cliCheckConnectionPeriodSecond,
+ ApiClientFactory clientFactory,
+ EventPublisher eventPublisher) {
+ this.clientFactory = clientFactory;
+ this.eventPublisher = eventPublisher;
+ this.cliCheckConnectionPeriodSecond = cliCheckConnectionPeriodSecond;
+ }
+
+ /**
+ * Starts connection heartbeat. By default connection will be checked every 5 sec.
+ *
+ * @param sessionInfo session info with node url
+ */
+ private void onConnect(SessionInfo sessionInfo) {
+ if (connected.compareAndSet(false, true)) {
+ eventPublisher.fireEvent(Events.connectionRestored());
+ }
+
+ if (scheduledConnectionHeartbeatExecutor == null) {
Review Comment:
I think a null check on `scheduledConnectionHeartbeatExecutor ` should be done under the lock in `onConnect` and in `onDisconnect` methods.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItConnectionHeartbeatTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.cli.commands;
+
+import static org.awaitility.Awaitility.await;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import io.micronaut.context.annotation.Property;
+import io.micronaut.context.annotation.Value;
+import jakarta.inject.Inject;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.ignite.internal.cli.core.repl.Session;
+import org.apache.ignite.internal.cli.event.EventFactory;
+import org.apache.ignite.internal.cli.event.EventListener;
+import org.apache.ignite.internal.cli.event.EventType;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+@Property(name="cli.check.connection.period.second", value="1")
+class ItConnectionHeartbeatTest extends CliCommandTestInitializedIntegrationBase {
+
+ @Inject
+ Session session;
+
+ @Inject
+ EventFactory eventFactory;
+
+ @Value("${cli.check.connection.period.second}")
+ private long CLI_CHECK_CONNECTION_PERIOD_SECONDS;
+
+ private final AtomicInteger connectionLost = new AtomicInteger(0);
+ private final AtomicInteger connectionRestored = new AtomicInteger(0);
+
+ @BeforeEach
+ void setUp() {
+ //ToDo: Set connection check timeout to 1 sec to make test fast
Review Comment:
Still here
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/EventFactory.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.cli.event;
+
+import jakarta.inject.Singleton;
+import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.ignite.internal.cli.logger.CliLoggers;
+import org.apache.ignite.internal.logger.IgniteLogger;
+
+/**
+ * Register listeners and produces events.
+ */
+@Singleton
+public class EventFactory implements EventPublisher, EventSubscriber {
+
+ private static final IgniteLogger log = CliLoggers.forClass(EventFactory.class);
+
+ /** All listeners. */
+ private final ConcurrentHashMap<EventType, List<EventListener>> listeners = new ConcurrentHashMap<>();
+
+ public EventFactory() {
+ }
+
+ /**
+ * Registers an event listener.
+ *
+ * @param eventType type of event to listen.
+ * @param eventListener event listener.
+ */
+ @Override
+ public void listen(EventType eventType, EventListener eventListener) {
Review Comment:
I think it is better to use "subscribe" everywhere. Sometimes you mix "subscribe" and "listen" but in fact, there is no difference.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander merged pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander merged PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1284538384
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/event/EventListener.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.cli.event;
+
+/**
+ * The listener handles events from a producer.
+ */
+@FunctionalInterface
+public interface EventListener {
+
+ void onEvent(EventType eventType, Event event);
Review Comment:
Could you add `eventType` method 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2359: IGNITE-19108 Check connection during REPL session in CLI
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2359:
URL: https://github.com/apache/ignite-3/pull/2359#discussion_r1284552609
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/SessionConnectEvent.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.cli.core.repl;
+
+import org.apache.ignite.internal.cli.event.Event;
+
+/**
+ * User session connected event.
+ */
+public class SessionConnectEvent implements Event {
+
+ private final SessionInfo sessionInfo;
+
+ public SessionConnectEvent(SessionInfo sessionInfo) {
+ this.sessionInfo = sessionInfo;
+ }
+
+ public SessionInfo getSessionInfo() {
Review Comment:
```suggestion
public SessionInfo sessionInfo() {
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org