-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[griddb] GridDB binding #1258
[griddb] GridDB binding #1258
Conversation
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.
any chance we could add tests?
Thank you for your time to review my pull request. |
I reflected all your comments. And I added a simple test code. |
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.
couple of quick changes and this should be good to merge.
looks like it needs a rebase, if you have a chance.
bin/ycsb
Outdated
@@ -76,6 +76,7 @@ DATABASES = { | |||
"geode" : "com.yahoo.ycsb.db.GeodeClient", | |||
"googlebigtable" : "com.yahoo.ycsb.db.GoogleBigtableClient", | |||
"googledatastore" : "com.yahoo.ycsb.db.GoogleDatastoreClient", | |||
"griddb" : "com.yahoo.ycsb.db.GridDBClient", |
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 should point to com.yahoo.ycsb.db.griddb.GridDBClient
now
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 changed package name for GridDB by 81cf64a .
|
||
LOGGER.info("notificationAddress=" + notificationAddress + " notificationPort=" + notificationPort + | ||
" notificationMember=" + notificationMember); | ||
LOGGER.info("clusterName=" + clusterName + " userName=" + userName + " password=" + password); |
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.
generally speaking, one should not log passwords.
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 cut password log by 86f7c71 .
I reflected your comments and run a rebase. |
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.
Please take a look at the Travis failures. It looks like you're missing test scope dependencies.
I'm sorry, but I forgot to update pom.xml. |
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.
the remaining test failure is a known problem with oracle jdk8 on travis that's fixed on current master branch.
thanks so much for all the work here @knonomura ! I'll send you a note when there's a release candidate with this binding ready for testing. |
Thank you very much for your review. |
I made a new pull request for adding GridDB binding to YCSB project.
GridDB (https://github.com/griddb/griddb_nosql) is a highly scalable NoSQL database best suited for IoT and Big Data.
Let me know if this is OK with you.
Thanks.