[cloudspanner] Add YCSB Client for Google's Cloud Spanner. by siamaktz · Pull Request #939 · brianfrankcooper/YCSB

siamaktz · 2017-03-04T00:12:17Z

No description provided.

Add Cloud Spanner client.

siamaktz · 2017-03-04T00:17:03Z

I'm a Google employee on the Cloud Spanner team and this is the official client that we would like to release. Thanks.

risdenk

Left some initial comments (package, naming, minor changes). I'll look at it again later this week.

risdenk · 2017-03-05T19:42:02Z

    "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",


Fix spacing here.

risdenk · 2017-03-05T19:43:06Z

+ * permissions and limitations under the License. See accompanying
+ * LICENSE file.
+ */
+package com.yahoo.ycsb.db;


Can this be placed in a cloudspanner package? This is something new that we are trying to do with all the bindings.

risdenk · 2017-03-05T19:44:13Z

+        public void run() {
+          System.out.println("Closing Cloud Spanner client...");
+          spanner.closeAsync();
+          System.out.println("...done.");


Standard error instead of standard out. Standard out is where measurements go.

Maybe use the logger instead of System.out?

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).

risdenk · 2017-03-05T19:45:42Z

    <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>


Is there a non beta version of the driver that can be used?

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.

risdenk · 2017-03-05T19:46:17Z

    <module>asynchbase</module>
    <module>azuretablestorage</module>
    <module>cassandra</module>
+    <module>cloudspanner</module>


Would be good to call this googlecloudspanner to match the other Google bindings.

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.

risdenk · 2017-03-05T19:47:35Z

+        throw new DBException(e);
+      }
+
+      LOGGER.log(Level.INFO, new StringBuilder()


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.

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.

risdenk · 2017-03-05T19:49:23Z

+ * The YCSB binding for Google's <a href="https://cloud.google.com/spanner/">
+ * Cloud Spanner</a>.
+ */
+package com.yahoo.ycsb.db;


Can you move this to a googlecloudspanner package? This is new trying to make all bindings in their own package.

risdenk · 2017-03-05T19:49:35Z

    <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>


Is there a non beta version of the driver?

risdenk · 2017-03-05T19:50:01Z

+        public void run() {
+          System.out.println("Closing Cloud Spanner client...");
+          spanner.closeAsync();
+          System.out.println("...done.");


This should probably use the logger instead of System.out?

Removed these as noted above.

risdenk · 2017-03-05T19:50:26Z

@@ -0,0 +1,11 @@
+# YCSB properties.


Missing license header? What is this used for anyway?

Just for the getting started example?

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.

siamaktz · 2017-03-13T21:20:46Z

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.

risdenk · 2017-03-14T13:47:03Z

@siamaktz thanks for the ping. I saw you updated just haven't had a chance to review it yet. Hopefully later this week.

risdenk · 2017-03-17T20:43:31Z

@siamaktz The changes look good to me. Thanks!

siamaktz · 2017-03-17T21:23:51Z

@risdenk Thank you for the review and for merging!

…per#939)

* 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)

siamaktz and others added 5 commits February 21, 2017 15:33

Add Cloud Spanner client.

62636cd

Add Cloud Spanner to distribution; make version a variable.

61d64cd

add default zeropadding and explain in documentation

fbe09db

reviewer comments; do not set host if not specified

a99ef86

Merge pull request #1 from mcurtiss/cloudspannerclient

4ef60f2

Add Cloud Spanner client.

siamaktz changed the title ~~Add YCSB Client for Google's Cloud Spanner.~~ [cloudspanner] Add YCSB Client for Google's Cloud Spanner. Mar 4, 2017

risdenk requested changes Mar 5, 2017

View reviewed changes

siamaktz added 3 commits March 7, 2017 18:22

fix whitespace; remove message when closing client

80be0be

Move CloudSpannerClient to a separate package called cloudspanner.

7587951

Add license info to properties file.

12fc22c

risdenk added the db-new label Mar 14, 2017

risdenk modified the milestone: 0.13.0 Mar 14, 2017

risdenk approved these changes Mar 17, 2017

View reviewed changes

risdenk merged commit 78c3cfa into brianfrankcooper:master Mar 17, 2017

siamaktz deleted the cloudspannerclient branch March 17, 2017 21:24

tzm41 pushed a commit to tzm41/YCSB that referenced this pull request May 7, 2017

[cloudspanner] Add binding for Google's Cloud Spanner. (brianfrankcoo…

1ff57a5

…per#939)

manolama mentioned this pull request Sep 22, 2017

Release 0.13.0 #981

Closed

busbey mentioned this pull request May 19, 2018

Release 0.14.0 #1117

Closed

Conversation

siamaktz commented Mar 4, 2017

Uh oh!

siamaktz commented Mar 4, 2017

Uh oh!

risdenk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siamaktz commented Mar 13, 2017

Uh oh!

risdenk commented Mar 14, 2017

Uh oh!

risdenk commented Mar 17, 2017

Uh oh!

siamaktz commented Mar 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants