[azuredocumentdb] Added support for Azure DocumentDB by k1xme · Pull Request #838 · brianfrankcooper/YCSB

k1xme · 2016-09-04T10:44:07Z

Support Azure DocumentDB.

scan operation is not supported currently.

risdenk

The changes look pretty good. I had a few minor comments. Can you squash your commits and start the commit with [documentdb]

risdenk · 2016-09-27T14:21:59Z

+  <artifactId>documentdb-binding</artifactId>
+  <name>DocumentDB Binding</name>
+
+  <properties>


Should be removed?

risdenk · 2016-09-27T14:22:29Z

@@ -0,0 +1,220 @@
+
+package com.yahoo.ycsb.db;


Can this be in a documentdb package? (com.yahoo.ycsb.db.documentdb)

risdenk · 2016-09-27T14:22:43Z

@@ -0,0 +1,48 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+Copyright (c) 2012 - 2016 YCSB contributors. All rights reserved.


Should be 2016

risdenk · 2016-09-27T14:22:52Z

@@ -0,0 +1,220 @@
+


Missing license header

risdenk · 2016-09-27T14:23:03Z

@@ -0,0 +1,22 @@
+/*
+ * Copyright 2015-2016 YCSB Contributors. All Rights Reserved.


should be 2016

risdenk · 2016-09-27T14:26:15Z

Looks like #837 has azure as well. What is the difference between table storage and documentdb?

risdenk · 2016-09-27T14:50:50Z

The binding should probably be something like azuredocumentdb

risdenk · 2016-10-31T20:31:09Z

@k1xme - any plans to update this PR?

k1xme · 2016-11-01T17:16:29Z

@risdenk Sorry! I totally forgot this. Will submit the changes by the end of this week.

[azuredocumentdb] Refactored code and added license header [azuredocumentdb] deleted unused class

k1xme · 2016-12-13T00:46:41Z

@risdenk Sorry for the late update. I've fixed the nits and style problem you mentioned. This PR is ready for integration now.

risdenk

Some of the package changes didn't seem to be complete. See comments.

risdenk · 2016-12-13T16:41:22Z

+/**
+ * The YCSB binding for Azure DocumentDB.
+ */
+package com.yahoo.ycsb.db;


package incorrect now?

also needs to be moved into the package folder

risdenk · 2016-12-13T16:41:52Z

+ * for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.yahoo.ycsb.db.azuredocumentdb;


Since the package was updated the file needs to be in the folder:
azuredocumentdb/src/main/java/com/yahoo/ycsb/db/azuredocumentdb/AzureDocumentDBClient.java

risdenk · 2016-12-13T16:42:21Z

 cassandra2-cql:com.yahoo.ycsb.db.CassandraCQLClient
 couchbase:com.yahoo.ycsb.db.CouchbaseClient
 couchbase2:com.yahoo.ycsb.db.couchbase2.Couchbase2Client
+azuredocumentdb:com.yahoo.ycsb.db.AzureDocumentDBClient


needs to be updated to new package location

risdenk · 2016-12-13T16:42:43Z

    "cassandra2-cql": "com.yahoo.ycsb.db.CassandraCQLClient",
    "couchbase"    : "com.yahoo.ycsb.db.CouchbaseClient",
    "couchbase2"   : "com.yahoo.ycsb.db.couchbase2.Couchbase2Client",
+    "documentdb"   : "com.yahoo.ycsb.db.DocumentDBClient",


needs to be updated to new package location and should be azuredocumentdb instead of just documentdb

k1xme · 2016-12-14T17:00:27Z

@risdenk Done!

risdenk

@k1xme I missed a few items from my previous review:

Error messages should go to stderr
checkstyle enforcement should be enabled
There is one error handling spot that just prints instead of exiting?

risdenk · 2016-12-14T19:23:46Z

+
+  <artifactId>azuredocumentdb-binding</artifactId>
+  <name>Azure DocumentDB Binding</name>
+  <properties>


This properties section should be removed. Any checkstyle errors should be fixed as well after this is removed.

risdenk · 2016-12-14T19:26:20Z

+                // TODO: Something has gone terribly wrong - the app wasn't
+                // able to query or create the collection.
+                // Verify your connection, endpoint, and key.
+                e.printStackTrace();


These errors should be sent to stderr. An example is here:

e.printStackTrace(System.err);

Another option would be to use a logging framework but that would be a bigger change.

This should be done for the other places w/ e.printStackTrace()

risdenk · 2016-12-14T19:27:25Z

+          // TODO: Something has gone terribly wrong - the app wasn't
+          // able to query or create the collection.
+          // Verify your connection, endpoint, and key.
+          e.printStackTrace();


Should this throw an exception or error? Instead of just printing the message?

k1xme · 2016-12-16T09:49:51Z

@risdenk fixed style check errors and exception handling nits per request.

risdenk

Thanks @k1xme

…r#838)

k1xme force-pushed the documentdb branch from cb17720 to 7091cf9 Compare September 4, 2016 12:00

risdenk requested changes Sep 27, 2016

View reviewed changes

risdenk mentioned this pull request Sep 27, 2016

[azuretablestorage] Add Azure table storage #837

Merged

risdenk added the db-new label Sep 27, 2016

risdenk changed the title ~~Added support for Azure DocumentDB~~ [azuredocumentdb] Added support for Azure DocumentDB Nov 28, 2016

k1xme added 3 commits December 12, 2016 23:57

Initial commit of DocumentDB

31233cf

Modified POM files to support documentdb

b848c82

Get flags from command line

fa62f19

k1xme force-pushed the documentdb branch from d71e7a2 to 6c52999 Compare December 13, 2016 00:02

[azuredocumentdb] Refactored Azure DocumentDB package

8d5c600

[azuredocumentdb] Refactored code and added license header [azuredocumentdb] deleted unused class

k1xme force-pushed the documentdb branch from 6c52999 to 8d5c600 Compare December 13, 2016 00:06

k1xme added 2 commits December 13, 2016 00:07

Merge remote-tracking branch 'upstream/master' into documentdb

f0f156e

[azuredocumentdb] fixed bug in distributions

0b1ab18

risdenk requested changes Dec 13, 2016

View reviewed changes

[azuredocumentdb] fixed nits

6a8eb30

risdenk requested changes Dec 14, 2016

View reviewed changes

k1xme added 2 commits December 16, 2016 09:18

[azuredocumentdb] more robust exception handling

2e23569

[azuredocumentdb] fixed style issues

379f811

risdenk approved these changes Dec 16, 2016

View reviewed changes

risdenk merged commit a37bc78 into brianfrankcooper:master Dec 16, 2016

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

[azuredocumentdb] Added support for Azure DocumentDB (brianfrankcoope…

02e27ef

…r#838)

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

		@@ -0,0 +1,22 @@
		/*
		* Copyright 2015-2016 YCSB Contributors. All Rights Reserved.

Conversation

k1xme commented Sep 4, 2016

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!

risdenk commented Sep 27, 2016

Uh oh!

risdenk commented Sep 27, 2016

Uh oh!

risdenk commented Oct 31, 2016

Uh oh!

k1xme commented Nov 1, 2016

Uh oh!

k1xme commented Dec 13, 2016

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!

k1xme commented Dec 14, 2016

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!

k1xme commented Dec 16, 2016

Uh oh!

risdenk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants