NebulaGraph Sessionpool #474

Nicole00 · 2022-09-26T15:13:13Z

No description provided.

codecov-commenter · 2022-09-27T02:22:11Z

Codecov Report

Base: 63.38% // Head: 62.94% // Decreases project coverage by -0.43% ⚠️

Coverage data is based on head (67d7d4f) compared to base (34b28de).
Patch coverage: 54.68% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #474      +/-   ##
============================================
- Coverage     63.38%   62.94%   -0.44%     
- Complexity      729      778      +49     
============================================
  Files            66       71       +5     
  Lines          3479     3735     +256     
  Branches        507      536      +29     
============================================
+ Hits           2205     2351     +146     
- Misses          928     1015      +87     
- Partials        346      369      +23

Impacted Files	Coverage Δ
...m/vesoft/nebula/client/graph/NebulaPoolConfig.java	`100.00% <ø> (ø)`
...com/vesoft/nebula/client/graph/net/Connection.java	`100.00% <ø> (ø)`
...va/com/vesoft/nebula/client/graph/net/Session.java	`48.93% <0.00%> (-1.07%)`	⬇️
...vesoft/nebula/client/graph/net/SyncConnection.java	`50.81% <0.00%> (-0.03%)`	⬇️
.../vesoft/nebula/client/graph/SessionPoolConfig.java	`46.93% <46.93%> (ø)`
...va/com/vesoft/nebula/client/graph/SessionPool.java	`56.32% <56.32%> (ø)`
.../com/vesoft/nebula/client/graph/NebulaSession.java	`68.18% <68.18%> (ø)`
...ient/graph/exception/BindSpaceFailedException.java	`100.00% <100.00%> (ø)`
...m/vesoft/nebula/client/graph/net/SessionState.java	`100.00% <100.00%> (ø)`
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…eSession

client/src/main/java/com/vesoft/nebula/client/graph/SessionPoolConfig.java

client/src/test/java/com/vesoft/nebula/client/graph/net/TestSessionPool.java

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

client/src/main/java/com/vesoft/nebula/client/graph/SessionPoolConfig.java

jinyingsunny · 2022-10-09T09:47:16Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+                }
+            }
+            // if session size is less than max size, get session from pool
+            if (sessionList.size() < maxSessionSize) {


我看没加锁，如果并发走到这里的话，可能会超过最大个数吗？

There's lock on the getSession method, so it's impossible to get here concurrently.

jinyingsunny · 2022-10-09T09:53:59Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+     */
+    private synchronized NebulaSession getSession() throws ClientServerIncompatibleException,
+            AuthFailedException, IOErrorException, BindSpaceFailedException {
+        int retry = 1;


请教下哈：这里的retry ，没看到哪里把他设置成0呢，有啥用？

请教下哈：这里的retry ，没看到哪里把他设置成0呢，有啥用？

retry-- : use the value of retry first, then retry minus 1.

这里的while (retry-- > 0) 是不是只循环了一次，并没有在sleep之后re-get

jinyingsunny · 2022-10-09T10:21:13Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+            useSpace(nebulaSession, null);
+            throw e;
+        }
+        useSpace(nebulaSession, resultSet);


useSpace 不用放在final里面么？

only when the code in try{} throws IOErrorException， we need to execute useSpace to release the session.
If it throws AuthFailedException, it means the session is not create successfully.

jinyingsunny · 2022-10-09T11:26:59Z

client/src/test/java/com/vesoft/nebula/client/graph/net/TestSessionPool.java

+
+        // test USE SPACE
+        try {
+            sessionPool.execute("USE TEST_USE_SPACE");


USE TEST_USE_SPACE这里，可以一个语句中执行多个use space么？比如：

USE SPACE1 xxx；USE SPACE2 xxx

The code didn't check this case, so this statement will be executed.

想到一种情况哈，如果用户没有这个space的权限，当前内核返回的错误信息能处理吧？--安琪反馈，会在result set中返回

USE SPACE1 xxx；USE SPACE2 xxx

如果第一个语句报错，第二个语句自动阻塞，不会执行了对吧？是内核处理的；已验证会报 SpaceNotFound

jinyingsunny · 2022-10-09T11:28:36Z

client/src/test/java/com/vesoft/nebula/client/graph/net/TestSessionPool.java

+        // NebulaPool init failed
+        List<HostAddress> addresses = Arrays.asList(new HostAddress(ip, 1000));
+        SessionPoolConfig config =
+                new SessionPoolConfig(addresses, "space", "user", "12345");


这个失败原因是端口号错误么？注释中可以明确下不？

yeah, I'll add some comments， thanks for your suggestions.

jinyingsunny · 2022-10-09T11:29:35Z

client/src/test/java/com/vesoft/nebula/client/graph/net/TestSessionPool.java

+        assert (!sessionPool.init());
+        sessionPool.close();
+
+        // set MinSessionSize 0, and init will return true even if the user is wrong


注释是不是要删掉或改一下？

jinyingsunny · 2022-10-09T11:33:55Z

client/src/test/java/com/vesoft/nebula/client/graph/net/TestSessionPool.java

+
+        // set MinSessionSize 0, and init will return true even if the user is wrong
+        config = new SessionPoolConfig(addresses, "space", "user", "nebula")
+                .setMinSessionSize(0);


最小线程池，如果允许设置成0的话，就说允许session是无效的对吧？貌似也可以

还是按我们最后定的设计来吧

client/src/test/java/com/vesoft/nebula/client/graph/net/TestSessionPool.java

jinyingsunny · 2022-10-09T14:45:31Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+                return;
+            }
+        }
+        releaseSession(nebulaSession);


我看明白了：
releaseSession是放回去；
nebulaSession.release();是销毁session pool；

nebulaSession.release() is signout.

jinyingsunny · 2022-10-09T14:47:04Z

client/src/test/java/com/vesoft/nebula/client/graph/net/TestSessionPool.java

+
+        // test USE SPACE
+        try {
+            sessionPool.execute("USE TEST_USE_SPACE");


想到一种情况哈，如果用户没有这个space的权限，当前内核返回的错误信息能处理吧？--安琪反馈，会在result set中返回

jinyingsunny

LGTM

BleakStone · 2022-10-19T07:20:55Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+     */
+    private synchronized NebulaSession getSession() throws ClientServerIncompatibleException,
+            AuthFailedException, IOErrorException, BindSpaceFailedException {
+        int retry = 1;


这里的while (retry-- > 0) 是不是只循环了一次，并没有在sleep之后re-get

BleakStone · 2022-10-19T07:26:40Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+
+        NebulaSession nebulaSession = new NebulaSession(connection, authResult.getSessionId(),
+                authResult.getTimezoneOffset(), state);
+        ResultSet result = nebulaSession.execute("USE " + spaceName);


"USE " + spaceName 需要加上反引号 `` 么，确保包含特殊字符的spaceName不会报错

"USE " + spaceName 需要加上反引号 `` 么，确保包含特殊字符的spaceName不会报错

是的，感谢提出

BleakStone · 2022-10-19T07:29:02Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+            resultSet = nebulaSession.executeWithParameter(stmt, parameterMap);
+
+            // re-execute for session error
+            if (isSessionError(resultSet)) {


如果stmt包含多个执行语句，由于没有事物，其中部分执行成功，部分失败，这里re-execute再次执行会不会造成影响

如果stmt包含多个执行语句，由于没有事物，其中部分执行成功，部分失败，这里re-execute再次执行会不会造成影响

只有发生Session相关的异常才会re-execute，此时语句是没有被执行的

BleakStone · 2022-10-19T07:29:33Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+     */
+    private void checkSession() {
+        for (NebulaSession nebulaSession : sessionList) {
+            if (nebulaSession.isIdle()) {


这里有没线程安全的问题呢

感谢提出，pr #479 中将NebulaSession的状态改为AtomicReference 原子变量。

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

HarrisChu · 2022-09-29T09:05:43Z

client/src/main/java/com/vesoft/nebula/client/graph/net/SyncConnection.java

+    @Override
+    public boolean ping(long sessionID) {
+        try {
+            execute(sessionID, "YIELD 1;");


if the sessionID in invalid, return true or false?

HarrisChu · 2022-09-29T09:12:06Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+
+        // re-execute for session error
+        if (isSessionError(resultSet)) {
+            sessionList.remove(nebulaSession);


how to drop the connection if the session is invalid

good catch, I'll add session.release.

HarrisChu · 2022-09-29T09:18:46Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+        stmtCheck(stmt);
+        checkSessionPool();
+        NebulaSession nebulaSession = getSession();
+        ResultSet resultSet = nebulaSession.getSession().execute(stmt);


should try the exception and finally use space

HarrisChu · 2022-09-29T09:24:56Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+     * close the session pool
+     */
+    public void close() {
+        if (isClosed.get()) {


is it thread safe? i.e. the space is dropped, and two session use space then call use space function

yeah, the type of isClosed is thread safe.

HarrisChu · 2022-09-29T09:37:12Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+    /**
+     * get the number of all Session
+     */
+    public int getSessionNums() {


thread safe?

yeah, the list is thread safe.

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

HarrisChu · 2022-09-29T09:41:46Z

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java

+            throw new IllegalArgumentException("statement is null.");
+        }
+
+        if (stmt.trim().toLowerCase().startsWith("use") && stmt.trim().split(" ").length == 2) {


use nba

if there're two space, split length is not 2?

yeah, not judge the use a; use b

Nicole00 added 3 commits September 26, 2022 23:10

NebulaGraph SessionPool

5a5c896

add Setter for sessionPoolConfig

348ad5f

fix checkstyle

e78b51f

Nicole00 added 7 commits September 27, 2022 14:59

remove fastjson's test scope

ce84bfb

add ping for real session id

8a7df26

add check for config

c6bf2d9

add health check and session num update, re-execute use space, releas…

03dd48b

…eSession

shutdown the schedule task when close pool

fd44b18

retry the execution for session error

342b1b0

add test for sessionPool

13ff18c

Nicole00 force-pushed the sessionpool branch from 521f794 to 13ff18c Compare September 28, 2022 08:56

Nicole00 added 3 commits September 28, 2022 19:23

add check for session pool config

6df5b1f

add more check for session pool and statement

a28f942

test for thread safe

193b058

Nicole00 added the wip label Sep 28, 2022

Nicole00 added 4 commits September 28, 2022 21:44

fix the SessionPool's state

eb2d781

Change static variable to instance variable

92dd5f9

close SessionPool after test

0a50ea3

revert SessionPool's state

327e10c

Nicole00 requested review from Aiee, HarrisChu and jinyingsunny September 29, 2022 02:31

Nicole00 removed the wip label Sep 29, 2022

Nicole00 added 2 commits September 29, 2022 13:59

remove synchronized for session state

1c1413c

upadte test for thread safe

0f6b674

Nicole00 force-pushed the sessionpool branch from b8772af to 0f6b674 Compare September 29, 2022 08:23

release NebulaSession when execute trigger exception

d6363ad

jinyingsunny reviewed Sep 30, 2022

View reviewed changes

Nicole00 commented Sep 30, 2022

View reviewed changes

client/src/main/java/com/vesoft/nebula/client/graph/SessionPool.java Outdated Show resolved Hide resolved

client/src/main/java/com/vesoft/nebula/client/graph/SessionPoolConfig.java Outdated Show resolved Hide resolved

Nicole00 added 4 commits October 2, 2022 21:19

remove connectionPool

ff18f84

remove useless function

bab94a5

remove useless configs

a25116d

fix state of NebulaSession for schedule task

e66c42a

jinyingsunny reviewed Oct 9, 2022

View reviewed changes

Nicole00 added 3 commits October 10, 2022 00:09

update minSessionSize

bfdbc56

add test for password changed

068edae

fix test

67d7d4f

jinyingsunny self-requested a review October 10, 2022 03:09

jinyingsunny approved these changes Oct 10, 2022

View reviewed changes

Nicole00 merged commit 96f77e8 into vesoft-inc:master Oct 10, 2022

abby-cyber self-assigned this Oct 13, 2022

abby-cyber added the doc affected PR: improvements or additions to documentation label Oct 13, 2022

abby-cyber mentioned this pull request Oct 13, 2022

Update 4.nebula-java-client.md vesoft-inc/nebula-docs-cn#2201

Merged

wey-gu mentioned this pull request Oct 15, 2022

Weekly Report 2022-10-14 vesoft-inc/nebula-community#138

Closed

BleakStone reviewed Oct 19, 2022

View reviewed changes

Nicole00 mentioned this pull request Oct 20, 2022

add session healthCheck time & use thread-safe state for session #479

Merged

HarrisChu reviewed Oct 21, 2022

View reviewed changes

Nicole00 mentioned this pull request Oct 25, 2022

Session使用问题 #473

Closed

NebulaGraph Sessionpool #474

NebulaGraph Sessionpool #474

Conversation

Nicole00 commented Sep 26, 2022

codecov-commenter commented Sep 27, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinyingsunny Oct 9, 2022 • edited

Choose a reason for hiding this comment

jinyingsunny Oct 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinyingsunny Oct 9, 2022 • edited

Choose a reason for hiding this comment

jinyingsunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 27, 2022 •

edited

jinyingsunny Oct 9, 2022 •

edited

jinyingsunny Oct 9, 2022 •

edited

jinyingsunny Oct 9, 2022 •

edited