Skip to content

Conversation

@liujiajun
Copy link
Contributor

@liujiajun liujiajun commented Jul 4, 2021

Providing a more fluent way to construct Session and SessionPool

@coveralls
Copy link

coveralls commented Jul 4, 2021

Coverage Status

Coverage increased (+0.05%) to 67.911% when pulling d119dff on liujiajun:builder into dbfb564 on apache:master.

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Job!
Only minor issues!

  1. As we can directly use sh sbin/start-cli.sh to connect server with default host "127.0.0.1" and default port "6667" using default user "root" with default password "root", So I think the host and port of session/sessionPool can also have default values. On the one hand, it is more convenient to test IoTDB locally, on the other hand, it makes the session/sessionPool interface consistent with CLI. BTW, it's my fault to make a wrong example. Actually, A built-in-class builder creation must be performed just like "Session.Builder.build()" rather than 'SessionBuilder.build()'
  2. Please update the docs in Programming-Native-API.md.
  3. After this PR has been merged, please send an email to the community telling everyone how the new session/sessionPool can be created now.

@OneSizeFitsQuorum OneSizeFitsQuorum added the Module - Java Client Native API (Session) label Jul 5, 2021
@liujiajun
Copy link
Contributor Author

Thanks for the comments! Have changed accordingly.

I removed the old construction method in the docs and also changed examples, so that new users can use builder as a preferred way.

Will blast out an email after this is merged~

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last minor issue!

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@wangchao316 wangchao316 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wangchao316 wangchao316 merged commit f35a087 into apache:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Module - Java Client Native API (Session)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants