korlov42 commented on code in PR #1004:
URL: https://github.com/apache/ignite-3/pull/1004#discussion_r951361850
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/session/Session.java:
##########
@@ -88,6 +88,11 @@ public SessionId sessionId() {
return sessionId;
}
+ /** Idle timeout. */
Review Comment:
```suggestion
/** Returns the duration in millis after which the session will be
considered expired if no one touched it in the middle. */
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/session/SessionManager.java:
##########
@@ -88,4 +131,19 @@ public SessionId createSession(
private SessionId nextSessionId() {
return new SessionId(UUID.randomUUID());
}
+
+ @Override
Review Comment:
could you please add the javadoc here and below?
##########
modules/api/src/main/java/org/apache/ignite/tx/TransactionException.java:
##########
@@ -38,4 +38,5 @@ public TransactionException(String message) {
public TransactionException(Throwable cause) {
super(cause);
}
+
Review Comment:
looks like unrelated change
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/session/SessionManagerTest.java:
##########
@@ -0,0 +1,128 @@
+package org.apache.ignite.internal.sql.engine.session;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.util.Map;
+import java.util.UUID;
+import javax.validation.constraints.AssertTrue;
+import org.apache.ignite.internal.sql.engine.property.PropertiesHolder;
+import org.apache.ignite.internal.sql.engine.property.Property;
+import org.apache.ignite.internal.testframework.IgniteTestUtils;
+import org.jetbrains.annotations.Nullable;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class SessionManagerTest {
Review Comment:
missed javadoc
##########
modules/api/src/main/java/org/apache/ignite/sql/Session.java:
##########
@@ -220,7 +220,15 @@ default long[] executeBatch(@Nullable Transaction
transaction, String dmlQuery,
* @param timeUnit Timeunit to convert timeout to.
* @return Default query timeout in the given timeunit.
*/
- long defaultTimeout(TimeUnit timeUnit);
+ long defaultQueryTimeout(TimeUnit timeUnit);
+
+ /**
+ * Return default session timeout.
+ *
+ * @param timeUnit Timeunit to convert timeout to.
+ * @return Default session timeout in the given timeunit.
+ */
+ long defaultSessionTimeout(TimeUnit timeUnit);
Review Comment:
I would highlight in the name that this is an _idle_ timeout
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.sql.api;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.sql.engine.AbstractBasicIntegrationTest;
+import org.apache.ignite.internal.sql.engine.SqlQueryProcessor;
+import org.apache.ignite.internal.sql.engine.session.SessionId;
+import org.apache.ignite.internal.sql.engine.session.SessionManager;
+import org.apache.ignite.internal.testframework.IgniteTestUtils;
+import org.apache.ignite.lang.IgniteException;
+import org.apache.ignite.sql.IgniteSql;
+import org.apache.ignite.sql.Session;
+import org.junit.jupiter.api.Test;
+
+/** Test common SQL API. */
+public class ItCommonApiTest extends AbstractBasicIntegrationTest {
+ protected SqlQueryProcessor queryProcessor() {
+ return (SqlQueryProcessor)((IgniteImpl)
CLUSTER_NODES.get(0)).queryEngine();
+ }
+
+ /**
+ * Gets the SQL API.
+ *
+ * @return SQL API.
+ */
+ protected IgniteSql igniteSql() {
+ return CLUSTER_NODES.get(0).sql();
+ }
+
+ @Override protected int nodes() {
+ return 1;
+ }
+
+ /** Check correctness of session expiration. */
+ @Test
+ public void testSessionExpiration() throws Exception {
+ long timeout = TimeUnit.SECONDS.toMillis(10); // time from
SessionManager.checkPeriod
+
+ IgniteSql sql = igniteSql();
+
+ var queryProc = queryProcessor();
+
+ SessionManager sessionManager =
IgniteTestUtils.getFieldValue(queryProc, "sessionManager");
Review Comment:
We need to stop messing around with component internals in e2e tests. I
believe this test could be rewritten in order to use QueryCursor
##########
modules/api/src/main/java/org/apache/ignite/sql/Session.java:
##########
@@ -220,7 +220,15 @@ default long[] executeBatch(@Nullable Transaction
transaction, String dmlQuery,
* @param timeUnit Timeunit to convert timeout to.
* @return Default query timeout in the given timeunit.
*/
- long defaultTimeout(TimeUnit timeUnit);
+ long defaultQueryTimeout(TimeUnit timeUnit);
+
+ /**
+ * Return default session timeout.
Review Comment:
Let's put a few words describing how this timeout will affect the enduser.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/session/SessionManager.java:
##########
@@ -20,32 +20,73 @@
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
import org.apache.ignite.internal.sql.engine.CurrentTimeProvider;
+import org.apache.ignite.internal.sql.engine.exec.LifecycleAware;
import org.apache.ignite.internal.sql.engine.property.PropertiesHolder;
+import org.apache.ignite.internal.thread.IgniteThread;
+import org.apache.ignite.internal.util.worker.IgniteWorker;
import org.jetbrains.annotations.Nullable;
/**
* A manager of a server side sql sessions.
*/
-public class SessionManager {
+public class SessionManager implements LifecycleAware {
+ /** Check period in ms. */
+ private static long checkPeriod = TimeUnit.SECONDS.toMillis(10);
+
+ private static final IgniteLogger LOG =
Loggers.forClass(SessionManager.class);
+
+ /** Active sessions. */
private final Map<SessionId, Session> activeSessions = new
ConcurrentHashMap<>();
+
private final CurrentTimeProvider timeProvider;
+ /** session expiration worker. */
+ private final IgniteWorker expirationWorker;
+
+ private final AtomicBoolean startedFlag = new AtomicBoolean(false);
+
+
/**
* Constructor.
*
+ * @param igniteInstanceName String igniteInstanceName
* @param timeProvider A time provider to use for session management.
*/
- public SessionManager(CurrentTimeProvider timeProvider) {
+ public SessionManager(String igniteInstanceName, CurrentTimeProvider
timeProvider) {
this.timeProvider = timeProvider;
+
+ expirationWorker = new IgniteWorker(LOG, igniteInstanceName,
"session_cleanup-thread", null) {
+ @Override
+ protected void body() throws InterruptedException {
+ while (!isCancelled()) {
+ blockingSectionBegin();
+ try {
+ Thread.sleep(checkPeriod);
+ } finally {
+ blockingSectionEnd();
+ }
+
+
Review Comment:
extra line break
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/session/SessionManager.java:
##########
@@ -20,32 +20,73 @@
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
import org.apache.ignite.internal.sql.engine.CurrentTimeProvider;
+import org.apache.ignite.internal.sql.engine.exec.LifecycleAware;
import org.apache.ignite.internal.sql.engine.property.PropertiesHolder;
+import org.apache.ignite.internal.thread.IgniteThread;
+import org.apache.ignite.internal.util.worker.IgniteWorker;
import org.jetbrains.annotations.Nullable;
/**
* A manager of a server side sql sessions.
*/
-public class SessionManager {
+public class SessionManager implements LifecycleAware {
+ /** Check period in ms. */
+ private static long checkPeriod = TimeUnit.SECONDS.toMillis(10);
+
+ private static final IgniteLogger LOG =
Loggers.forClass(SessionManager.class);
+
+ /** Active sessions. */
private final Map<SessionId, Session> activeSessions = new
ConcurrentHashMap<>();
+
private final CurrentTimeProvider timeProvider;
+ /** session expiration worker. */
+ private final IgniteWorker expirationWorker;
+
+ private final AtomicBoolean startedFlag = new AtomicBoolean(false);
+
Review Comment:
extra line break
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -131,6 +132,8 @@ public SqlQueryProcessor(
this.schemaManager = schemaManager;
this.dataStorageManager = dataStorageManager;
this.dataStorageFieldsSupplier = dataStorageFieldsSupplier;
+
+ sessionManager = registerService(new
SessionManager(igniteInstanceName, System::currentTimeMillis));
Review Comment:
As I can see, current approach is to create LifecycleAware objects during
start phase. Does it make sense to follow this to make code more consistent?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/session/SessionManager.java:
##########
@@ -20,32 +20,73 @@
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
import org.apache.ignite.internal.sql.engine.CurrentTimeProvider;
+import org.apache.ignite.internal.sql.engine.exec.LifecycleAware;
import org.apache.ignite.internal.sql.engine.property.PropertiesHolder;
+import org.apache.ignite.internal.thread.IgniteThread;
+import org.apache.ignite.internal.util.worker.IgniteWorker;
import org.jetbrains.annotations.Nullable;
/**
* A manager of a server side sql sessions.
*/
-public class SessionManager {
+public class SessionManager implements LifecycleAware {
+ /** Check period in ms. */
+ private static long checkPeriod = TimeUnit.SECONDS.toMillis(10);
Review Comment:
While 10 seconds looks good enough default value, this setting should find
its way into the configuration. But for now I would suggest to reduce this to 1
second or even to a half of a second to shorten the time of related tests. WDYT?
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/session/SessionManagerTest.java:
##########
@@ -0,0 +1,128 @@
+package org.apache.ignite.internal.sql.engine.session;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.util.Map;
+import java.util.UUID;
+import javax.validation.constraints.AssertTrue;
+import org.apache.ignite.internal.sql.engine.property.PropertiesHolder;
+import org.apache.ignite.internal.sql.engine.property.Property;
+import org.apache.ignite.internal.testframework.IgniteTestUtils;
+import org.jetbrains.annotations.Nullable;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class SessionManagerTest {
+
+ private SessionManager sessionMgr;
+ private Map<SessionId, Session> activeSessions;
+
+ @BeforeEach
+ void beforeEach() {
+ sessionMgr = new SessionManager("test", System::currentTimeMillis);
+ activeSessions = IgniteTestUtils.getFieldValue(sessionMgr,
"activeSessions");
+ }
+
+ @AfterEach
+ void afterEach() throws Exception {
+ sessionMgr.stop();
+ }
+
+ @Test
+ void createSession() {
+ assertEquals(0, activeSessions.size());
+
+ SessionId sessionId1 = sessionMgr.createSession(1000, null);
+ assertNotNull(sessionId1);
+
+ SessionId sessionId2 = sessionMgr.createSession(1000, null);
+ assertNotNull(sessionId2);
+
+ assertNotEquals(sessionId1, sessionId2);
+ assertEquals(2, activeSessions.size());
+ }
+
+ @Test
+ void sessionGet() {
+ PropertiesHolder propHldr = createPropertyHolder();
+
+ SessionId sessionId = sessionMgr.createSession(12345, propHldr);
+
+ Session session = sessionMgr.session(sessionId);
+ assertNotNull(session);
+ assertSame(propHldr, session.queryProperties());
+ assertEquals(12345, session.getIdleTimeoutMs());
+
+ SessionId unknownSessionId = new SessionId(UUID.randomUUID());
+ assertNull(sessionMgr.session(unknownSessionId));
+ }
+
+ @Test
+ void touchSessionDuringGet() throws InterruptedException {
Review Comment:
It gets tricky when it comes to testing the time related things. That's why
I have introduced CurrentTimeProvider interface to be used in
SessionManagement. By using this interface you could travel backward and
forward in time without wasting time in sleeping.
So please get rid of all `Thread.sleep()` in the class
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/session/SessionManagerTest.java:
##########
@@ -0,0 +1,128 @@
+package org.apache.ignite.internal.sql.engine.session;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.util.Map;
+import java.util.UUID;
+import javax.validation.constraints.AssertTrue;
+import org.apache.ignite.internal.sql.engine.property.PropertiesHolder;
+import org.apache.ignite.internal.sql.engine.property.Property;
+import org.apache.ignite.internal.testframework.IgniteTestUtils;
+import org.jetbrains.annotations.Nullable;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class SessionManagerTest {
+
+ private SessionManager sessionMgr;
+ private Map<SessionId, Session> activeSessions;
+
+ @BeforeEach
+ void beforeEach() {
+ sessionMgr = new SessionManager("test", System::currentTimeMillis);
+ activeSessions = IgniteTestUtils.getFieldValue(sessionMgr,
"activeSessions");
+ }
+
+ @AfterEach
+ void afterEach() throws Exception {
+ sessionMgr.stop();
+ }
+
+ @Test
+ void createSession() {
Review Comment:
I'm not sure this test is useful. Why do you think we need to verify that
size of some internal collection correlates to amount of invocation of some
method?
To me, the only thing matters here is that you are able to retrieve session
by returned identifier
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/session/SessionManager.java:
##########
@@ -88,4 +131,19 @@ public SessionId createSession(
private SessionId nextSessionId() {
return new SessionId(UUID.randomUUID());
}
+
+ @Override
+ public synchronized void start() {
Review Comment:
why do you need this to be synchronised?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/session/SessionManager.java:
##########
@@ -20,32 +20,73 @@
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
import org.apache.ignite.internal.sql.engine.CurrentTimeProvider;
+import org.apache.ignite.internal.sql.engine.exec.LifecycleAware;
import org.apache.ignite.internal.sql.engine.property.PropertiesHolder;
+import org.apache.ignite.internal.thread.IgniteThread;
+import org.apache.ignite.internal.util.worker.IgniteWorker;
import org.jetbrains.annotations.Nullable;
/**
* A manager of a server side sql sessions.
*/
-public class SessionManager {
+public class SessionManager implements LifecycleAware {
+ /** Check period in ms. */
+ private static long checkPeriod = TimeUnit.SECONDS.toMillis(10);
Review Comment:
Also let's move this constant to SqlQueryProcessor and pass it as a
constructor argument. This will 1) gather all configuration params in a single
place and 2) make it easy to test SessionManager
--
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]
