Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add session manager #217

Closed
wants to merge 4 commits into from
Closed

Conversation

horoc
Copy link

@horoc horoc commented Aug 10, 2022

refer to the issue
#215

I have tried to implement a session manager refer to the solution of java client, which mainly aim to improve performance in concurrent query.

In current go-client:

  1. we need to append "USER SPACE" prefix in each query statement, it's very inconvenient and also bring performance overheads.
  2. the GetSession function of ConnectionPool will send authenticate request to check username and password every time, it may also affect the performance in concurrent query.

according to the reasons above, I have tried to implement a session manager and do some simple benchmark test:

env : 16 core cpu, 32GB memory
dataset: https://docs.nebula-graph.com.cn/3.1.0/2.quick-start/4.nebula-graph-crud/

Concurrent query with "GO FROM" statment:

with session manager :

function :  session_manager_test.go:56
goos: darwin
goarch: arm64
pkg: github.com/vesoft-inc/nebula-go/v3
BenchmarkSessionManager-10         10000            495182 ns/op
PASS
ok      github.com/vesoft-inc/nebula-go/v3      6.222s

only connection pool :

session_manager_test.go:84
goos: darwin
goarch: arm64
pkg: github.com/vesoft-inc/nebula-go/v3
BenchmarkSessionPool-10            10000          13417409 ns/op
PASS
ok      github.com/vesoft-inc/nebula-go/v3      134.484s

I think maybe it's useful for some high QPS of latency sensitive scenarios.

@CLAassistant
Copy link

CLAassistant commented Aug 10, 2022

CLA assistant check
All committers have signed the CLA.

@wey-gu wey-gu requested a review from Aiee August 11, 2022 01:34
@wey-gu wey-gu mentioned this pull request Aug 16, 2022
@Aiee
Copy link
Contributor

Aiee commented Aug 17, 2022

Thanks for your contribution. According to the feedback we received from the java client, we might need a different design of the session pool. For example, we can execute a query via the session pool without getting a session wrapper instance. A PR address this problem will be submitted later, please keep an eye on it.

@wey-gu
Copy link
Contributor

wey-gu commented Aug 17, 2022

refer to the issue #215

I have tried to implement a session manager refer to the solution of java client, which mainly aim to improve performance in concurrent query.

In current go-client:

  1. we need to append "USER SPACE" prefix in each query statement, it's very inconvenient and also bring performance overheads.
  2. the GetSession function of ConnectionPool will send authenticate request to check username and password every time, it may also affect the performance in concurrent query.

according to the reasons above, I have tried to implement a session manager and do some simple benchmark test:

env : 16 core cpu, 32GB memory dataset: https://docs.nebula-graph.com.cn/3.1.0/2.quick-start/4.nebula-graph-crud/

Concurrent query with "GO FROM" statment:

with session manager :

function :  session_manager_test.go:56
goos: darwin
goarch: arm64
pkg: github.com/vesoft-inc/nebula-go/v3
BenchmarkSessionManager-10         10000            495182 ns/op
PASS
ok      github.com/vesoft-inc/nebula-go/v3      6.222s

only connection pool :

session_manager_test.go:84
goos: darwin
goarch: arm64
pkg: github.com/vesoft-inc/nebula-go/v3
BenchmarkSessionPool-10            10000          13417409 ns/op
PASS
ok      github.com/vesoft-inc/nebula-go/v3      134.484s

I think maybe it's useful for some high QPS of latency sensitive scenarios.

Hi @horoc, we appreciated your contributions and continually working at night for this elegant implementation, extremely sorry for how it goes as so due to the community is planning on a whole new design.

While an unmerged PR is as valuable (especially for those like yours) as those merged, we will reach out to you via mail (gmail) for Contributor memorabilia and certificates, kindly help reply the mail with your address :)

It's great to have you in the community @horoc .

Cheers!

@horoc
Copy link
Author

horoc commented Aug 18, 2022

refer to the issue #215
I have tried to implement a session manager refer to the solution of java client, which mainly aim to improve performance in concurrent query.
In current go-client:

  1. we need to append "USER SPACE" prefix in each query statement, it's very inconvenient and also bring performance overheads.
  2. the GetSession function of ConnectionPool will send authenticate request to check username and password every time, it may also affect the performance in concurrent query.

according to the reasons above, I have tried to implement a session manager and do some simple benchmark test:
env : 16 core cpu, 32GB memory dataset: https://docs.nebula-graph.com.cn/3.1.0/2.quick-start/4.nebula-graph-crud/
Concurrent query with "GO FROM" statment:
with session manager :

function :  session_manager_test.go:56
goos: darwin
goarch: arm64
pkg: github.com/vesoft-inc/nebula-go/v3
BenchmarkSessionManager-10         10000            495182 ns/op
PASS
ok      github.com/vesoft-inc/nebula-go/v3      6.222s

only connection pool :

session_manager_test.go:84
goos: darwin
goarch: arm64
pkg: github.com/vesoft-inc/nebula-go/v3
BenchmarkSessionPool-10            10000          13417409 ns/op
PASS
ok      github.com/vesoft-inc/nebula-go/v3      134.484s

I think maybe it's useful for some high QPS of latency sensitive scenarios.

Hi @horoc, we appreciated your contributions and continually working at night for this elegant implementation, extremely sorry for how it goes as so due to the community is planning on a whole new design.

While an unmerged PR is as valuable (especially for those like yours) as those merged, we will reach out to you via mail (gmail) for Contributor memorabilia and certificates, kindly help reply the mail with your address :)

It's great to have you in the community @horoc .

Cheers!

Thanks for the reply!
Since we are trying to integrate nebula with our project, we are trying to reuse session in a more effective way, that's the background of this PR. And we are happy to see the community planning to redesign the session pool, and look forward to the update. I will close the PR, thanks for your help. btw, my email address : horoc.chen at gmail dot com.

@horoc horoc closed this Aug 18, 2022
@Aiee
Copy link
Contributor

Aiee commented Sep 16, 2022

Hi @horoc
We have the new session pool here #222. Please feel free to leave feedback and reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants