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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
