-
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
[cloudspanner] Add YCSB Client for Google's Cloud Spanner. #939
Conversation
Add Cloud Spanner client.
I'm a Google employee on the Cloud Spanner team and this is the official client that we would like to release. Thanks. |
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.
Left some initial comments (package, naming, minor changes). I'll look at it again later this week.
bin/ycsb
Outdated
@@ -59,6 +59,7 @@ DATABASES = { | |||
"basic" : "com.yahoo.ycsb.BasicDB", | |||
"cassandra-cql": "com.yahoo.ycsb.db.CassandraCQLClient", | |||
"cassandra2-cql": "com.yahoo.ycsb.db.CassandraCQLClient", | |||
"cloudspanner" : "com.yahoo.ycsb.db.CloudSpannerClient", |
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.
Fix spacing here.
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.
Done.
* permissions and limitations under the License. See accompanying | ||
* LICENSE file. | ||
*/ | ||
package com.yahoo.ycsb.db; |
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 this be placed in a cloudspanner package? This is something new that we are trying to do with all the bindings.
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.
Done.
public void run() { | ||
System.out.println("Closing Cloud Spanner client..."); | ||
spanner.closeAsync(); | ||
System.out.println("...done."); |
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.
Standard error instead of standard out. Standard out is where measurements go.
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.
Maybe use the logger instead of System.out?
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 removed these as they are not really necessary at all. FWIW, when using the logger, the message are not actually realized on the screen (the program exits before that has a chance to happen).
@@ -100,6 +100,7 @@ LICENSE file. | |||
<arangodb.version>2.7.3</arangodb.version> | |||
<arangodb3.version>4.1.7</arangodb3.version> | |||
<azurestorage.version>4.0.0</azurestorage.version> | |||
<cloudspanner.version>0.9.3-beta</cloudspanner.version> |
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.
Is there a non beta version of the driver that can be used?
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.
No, Cloud Spanner is only released as beta so far. Once we go to a GA release, we will update the version to match the release.
@@ -114,6 +115,7 @@ LICENSE file. | |||
<module>asynchbase</module> | |||
<module>azuretablestorage</module> | |||
<module>cassandra</module> | |||
<module>cloudspanner</module> |
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.
Would be good to call this googlecloudspanner to match the other Google bindings.
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 discussed this internally within Google and the decision was that the official name of the product is "Cloud Spanner" rather than "Google Cloud Spanner" and thus we'd like it to be called just "cloudspanner" here.
throw new DBException(e); | ||
} | ||
|
||
LOGGER.log(Level.INFO, new StringBuilder() |
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 make sure this logging goes to stderr? Stdout is used for printing measurements. This makes it easy to split measurements from log messages.
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 double checked that all logging goes to stderr by default. So unless people explicitly provide logging options to make these go to stdout, it should be fine.
* The YCSB binding for Google's <a href="https://cloud.google.com/spanner/"> | ||
* Cloud Spanner</a>. | ||
*/ | ||
package com.yahoo.ycsb.db; |
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 you move this to a googlecloudspanner package? This is new trying to make all bindings in their own package.
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.
Done.
@@ -100,6 +100,7 @@ LICENSE file. | |||
<arangodb.version>2.7.3</arangodb.version> | |||
<arangodb3.version>4.1.7</arangodb3.version> | |||
<azurestorage.version>4.0.0</azurestorage.version> | |||
<cloudspanner.version>0.9.3-beta</cloudspanner.version> |
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.
Is there a non beta version of the 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.
See above.
public void run() { | ||
System.out.println("Closing Cloud Spanner client..."); | ||
spanner.closeAsync(); | ||
System.out.println("...done."); |
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 probably use the logger instead of System.out?
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.
Removed these as noted above.
@@ -0,0 +1,11 @@ | |||
# YCSB properties. |
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 license header? What is this used for anyway?
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.
Just for the getting started example?
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.
Thanks for noticing this, I added the license header. Yes, it is used in the README and more generally provides a set of recommended options, i.e. we'd like that people use this as a base for their own properties in their experiments.
Friendly ping. I tried to address the raised issues and comments and I'm looking forward to the next step towards integrating this into the main branch. Thanks. |
@siamaktz thanks for the ping. I saw you updated just haven't had a chance to review it yet. Hopefully later this week. |
@siamaktz The changes look good to me. Thanks! |
@risdenk Thank you for the review and for merging! |
* master: [core] Fixing squid:S1319 - Declarations should use Java collection interfaces such as "List" rather than specific implementation classes such as "LinkedList". (manolama - updated bindings added since the PR) [core] Use longs instead of ints to support larger key spaces. Changed int to long in Measurements code to support large scale workloads. (manolama - fixed checkstyle errors) [core] Export totalHistogram for HdrHistogram measurement [core] Add an operation enum to the Workload class. This can eventually be used to replace the strings. [core] Add a Fisher-Yates array shuffle to the Utils class. [core] Fix an issue where the threadid and threadCount were not passed to the workload client threads. Had to use setters to get around the checkstyle complaint of having too many parameters. Upgrading googlebigtable to the latest version. The API used by googlebigtable has had quite a bit of churn. This is the minimal set of changes required for the upgrade. [geode] Update to apache-geode 1.2.0 release [core] Update to use newer version of Google Cloud Spanner client and associated required change [core] Add a reset() method to the ByteIterator abstract and implementations for each of the children. This lets us re-use byte iterators if we need to access the values again (when applicable). [hbase12] Add HBase 1.2+ specific client that relies on the shaded client artifact provided by those versions. (brianfrankcooper#970) [distro] Refresh Apache licence text (brianfrankcooper#969) [memcached] support binary protocol (brianfrankcooper#965) [accumulo] A general "refresh" to the Accumulo binding (brianfrankcooper#947) [cloudspanner] Add binding for Google's Cloud Spanner. (brianfrankcooper#939) [aerospike] Change the write policy to REPLACE_ONLY (brianfrankcooper#937)
No description provided.