-
Notifications
You must be signed in to change notification settings - Fork 21
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
A client for Nebula Graph Database that support multi versions #59
Conversation
02270c1
to
ba842f6
Compare
8e3f57b
to
0fc1da0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ccore/nebula/options.go
Outdated
return func(o *Options) { | ||
WithGraphTLS(tlsConfig) | ||
WithGraphTLS(tlsConfig) | ||
WithGraphTLS(tlsConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing WithStorageTLS and WithMetaTLS ?
Client interface { | ||
Graph() GraphClient | ||
Meta() MetaClient | ||
StorageAdmin() StorageAdminClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we call storageAdmin storage? for users, they don't know what is storageAdmin, but use it for storage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is named according to thrift
define, it is best not to introduce new terms.
|
||
if o.log == nil { | ||
o.log = defaultOpts.log | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't use if o.log == nil { o.log = noOpLogger{} }
instead, don't need define defaultOpts
and check o.version same as o.log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the same entrance defaultOptions
. If this future changes, only defaultOptions
need to change.
V2_0_0 = types.V2_0_0 | ||
V2_5_0 = types.V2_5_0 | ||
V2_5_1 = types.V2_5_1 | ||
V2_6_0 = types.V2_6_0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/vesoft-inc/nebula-go/pull/148/files, I see thrift code use 2.6.0, so we need fix this compile bug too, or we use 2.6.1 thrift code instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed.
} | ||
|
||
func (c *defaultMetaClient) Close() error { | ||
return c.meta.open(c.driver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use c.meta.close()
?
ccore/nebula/client_graph.go
Outdated
} | ||
|
||
func (c *defaultGraphClient) Close() error { | ||
return c.graph.open(c.driver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use c.graph.close()
?
ccore/nebula/client_storage_admin.go
Outdated
} | ||
|
||
func (c *defaultStorageAdminClient) Close() error { | ||
return c.storageAdmin.open(c.driver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use c.storageAdmin.close()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
A client for
Nebula Graph Database
that support multi versions.Those files in
ccore/nebula/internal/thrift
is copy from https://github.com/vesoft-inc/nebula-go .