nebula-contrib / nebula-node

Nebula Graph Client for Node.js
26 stars 9 forks source link

Fix ping #12

Closed jjsimps closed 2 years ago

jjsimps commented 2 years ago

I was getting invalid session errors in the driver during periods of zero-activity. Looking into the driver, the ping function does not use the correct session id. Now it works and no longer throws error -1002. Also improved error reporting if something goes wrong during ping.

wey-gu commented 2 years ago

Awesome! Thanks @jjsimps !

jjsimps commented 2 years ago

Hi, can this be reviewed please? @wujjpp . Would love to have this fixed & released. Thanks!

wujjpp commented 2 years ago

@jjsimps the ping logical from python SDK located at https://github.com/vesoft-inc/nebula-python/blob/HEAD/nebula3/gclient/net/Connection.py#L219

it seems 0 can be sessionId during executing YIELD 1 and the ping is used for detecting network connective, if diconnect unexpect it will cause error and then do reconnecting.

wey-gu commented 2 years ago

Thanks, @wujjpp, it turned out the session-id 0 was used based on an assumption/agreement that it's an invalid id only for probing the connection(as there is no such thing in thrift).

What is the nebula graph server version are you using now, please?

@Aiee do you have ideas on @jjsimps' observation, please?

jjsimps commented 2 years ago

I am running on nebula version 3.1.0

For reference, if I modify the code to print the response:

.execute(0, 'YIELD 1')
.then((r) => {
  console.log(r)
  clearTimeout(timer)
  resolve(true)
})

I get:

{
  error_code: -1002,
  latency_in_us: null,
  data: null,
  space_name: null,
  error_msg: 'Invalid session id',
  plan_desc: null,
  comment: null
}

So yeah...not sure why this isn't working then if 0 is expected. Please note that no exception is thrown in this case -- the error in part of the response fields. Also to be clear I am hitting session_idle_timeout_secs because of no queries on the connection.

jjsimps commented 2 years ago

Thank you! Would you know when you'd be able to release the changes to npm?

wey-gu commented 2 years ago

Thank you! Would you know when you'd be able to release the changes to npm?

I had built and released one minor version to npmjs as 2.6.2 just now, I don't have time to make it automated but manually for now :P