Client API design and code

[sirkon]

Hi, no offence, but your code is utterly non-idiomatic. Completely not up to the DB itself: I like Nebula, but your Go code – not so much. I will split issues into two kinds:

Code style and layout issues

• It is not formatted with gofmt. I understand you may not like formatting with tabs, but this is how it is written in Go. Please apply gofmt and set up your editor to apply it on save. This is very basic, code formatted like yours will be immediately rejected if you would try to contribute in any barely decent project.

• Library layout. It is not recommended in Go to split public functionality in different packages as it makes totally unclear what repo this package belongs to, as Go does not have nested namespaces. Use prefixes in a single package instead. You may split a functionality into different packages, but makes this in internal folder of you package to hide them away from outer users and use type aliases type ClientConnection = client.Connection and wrap function calls in the package itself:

  …
  import "github.com/vesoft-inc/internal/client"
  
  type Client = client.Client
  
  func NewClient(addr string) (Client, error) {
      return client.New(addr)
  }

Code issues

• There're 3 popular logging options in Go:

  • logrus
  • zap
  • zerolog

  Stdlib logger is not really a common option in any half-serious project. one of these three is typically a better pick. I mean, a project using your DB is likely to be a part of large infrastructure and they are very likely to use structured logging any of these packages provide. Your logging not only will be alien, the lines printed in stdlib logger will likely be missing important things like trace-request-id, etc. You should not use logging in packages and should return errors instead which will be printed with a logger of choice.

• Error processing is totally off.

  • Splitting error processing into two parts is insane for public API. You should wrap response errors in Go error. Just implement your own error type and do response processing yourself:

    type ErrorResponse struct {
        code graph.ErrorCode
        msg string
    }
    
    func newErrorResponse(code *graph.ErrorCode, msg string) *ErrorResponse {
        return &ErrorResponse{
            code: code,
            msg: msg,
        }
    }
    
    func (e *ErrorResponse) Error() string {
        return fmt.Sprintf("%s: %s", e.code, e.msg)
    }
    
    …
    // hijack response error in Execute method
    if IsError(resp) {
        return nil, newErrorResponse(resp.GetErrorCode(), resp.GetErrorMsg())
    }

    Go users will be able to understand if it is some low level network error or Nebula error with errors.As(err, &errRespVar) if needed (I doubt if it is really needed though)

• You should use errors wrapping instead of logging:

  • Wrong

    if err != nil {
        log.Printf("failed to do something: %s", err)
        return
    }

  • Right

    if err != nil {
        return fmt.Errorf("do something: %w", err)
    }

In the end users will have error chain annotated with steps what lead to the error. They will be able to extract root error with errors.As if needed. The error text itself will be self-explaining, showing a chain and error context what you can put in an error.

[Aiee]

Hi sirkon,

Thank you for your time and your insightful suggestions for Nebula go client, and I am sorry for any inconvenience you had when using go client. You are right, back in the days when the dev team was lack of experience coding in golang, we didn't polish it well.

As the release date of Nebula 2.0 is approaching, we have redesigned the client, with the concerns we hear from the users. I will give you a brief introduction of the changes we made regarding the suggestions you gave:

• Code style and layout issues
  • Everything is formatted now!
  • Libraries are reorganized now so that the only module the user needs to import is nebula "github.com/vesoft-inc/nebula-go", which simplifies the development for users.
• Code issues

  • Logging:

    • Choosing a logger to use as the default client log may cause duplicated logs when users use a different logger. Since we don't know what loggers our users are using, we decided to create a logger interface that allows the user to implement the method in the interface with their preference of logger choice.

  • Error processing:

    • We have refactored the error processing. A typical error handling case will be like:

    // Create session
    session, err := pool.GetSession(username, password)
    if err != nil {
      log.Fatal(fmt.Sprintf("Fail to create a new session from connection pool, username: %s, password: %s, %s",
    	  username, password, err.Error()))
    }
    

Again, I want to thank you for using Nebula and being a part of the community. Your feedback is important to us. Please let us know if there is anything else you think is off or we can improve on.

Have a nice day!