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

jdbc URL string support #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

jdbc URL string support #34

wants to merge 1 commit into from

Conversation

wey-gu
Copy link
Member

@wey-gu wey-gu commented Sep 5, 2022

As discussed in #27

@wey-gu
Copy link
Member Author

wey-gu commented Sep 5, 2022

f548602 is the change proposed by @corradomio

This PR is easier for contributors to co-author and collaboration on bringing standard URL string support for nebula-jdbc.

@wey-gu
Copy link
Member Author

wey-gu commented Sep 5, 2022

cc @Young-Flash @Nicole00

@wey-gu wey-gu changed the title Initial change provided by @corradomio jdbc URL string support Sep 5, 2022
@wey-gu
Copy link
Member Author

wey-gu commented Sep 5, 2022

We need help to review/polish/refactor this change.

}

// close the current pool that uses '127.0.0.1' (WHY????)
this.nebulaPool.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it should be refactored.
If url is the param of connect, then when get driver, there's should't init the pool, just init the pool in connect function.
Or give the url to driver, and init the connection pool in getDriver function.

The default client address 127.0.0.1 is unreasonable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

totally agreed, would be better to remove this one.

Copy link

Choose a reason for hiding this comment

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

@Nicole00 I'm sorry but i saw line 180 the customizedAddressList will replace the poolProperties's addressList , so in my opinion the default IP 127.0.0.1 will by replaced by the customizedAddressList ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it will be replaced by customizedAddressList only when user call new NebulaDriver(address), but it's not the standard way to use jdbc. Almost usage is Class.forName() and it will call the empty construct function, so the address will be 127.0.0.1.

We can remove the customizedAddressList and put the user address in url, which will be more common.

@xiajingchun
Copy link
Collaborator

Another thing is, using the common way to load driver class won't get it registered yet.
Need to add to NebulaDriver some static code to call DriverManager.registerDriver(), in addtion to the constructor.

@wey-gu wey-gu added the help wanted Community: does anyone want to work on it? label Sep 9, 2022
for (String address : addressesList) {
String[] addressInfo = address.split(":");
if(addressInfo.length != 2){
throw new SQLException(String.format("url [%s] is invalid, please make sure your url match thr format: \"ip:port\".", url));

Choose a reason for hiding this comment

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

"thr" seems to be a typo?

Copy link
Member Author

@wey-gu wey-gu Sep 9, 2022

Choose a reason for hiding this comment

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

Ha, thanks! Should be /thr/the/😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Community: does anyone want to work on it?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants