[jdbc] Assorted cleanup/fixes around batching and supporting different databases #842

joshelser · 2016-09-20T01:52:46Z

A couple of things included in this PR. Happy to split them out if that is preferred. I just came across all of these in some recent testing.

Builds on @enis's commit which didn't make it upstream. Implements a "flavor" for Apache Phoenix (which only provides UPSERT to write data, no INSERT)
Some basic cleanup of JdbcDBClient (rip out all of the flavors logic into their own files)
De-couples batching APIs and autocommit. Again, trying to support Phoenix better, but not break JDBC semantics.
Finally, fixes an issue with the existing batching support where the CoreWorkload treated Status.BATCHED_OK as an error (not the same as Status.OK)

joshelser · 2016-09-20T01:53:06Z

@busbey this is the changeset I was referring to earlier today in private chat.

joshelser · 2016-09-20T02:40:37Z

Also, I forgot to mention the testing I had done..

Verified that both thick and thin JDBC drivers work with expected performance after these changes
MySQL works before and after changes with similar perf (workloada)

risdenk

Overall the changes look good. There are a few minor copyright things that should probably be addressed.

risdenk · 2016-09-27T03:07:43Z

jdbc/src/main/java/com/yahoo/ycsb/db/flavors/PhoenixDBFlavor.java

@@ -0,0 +1,65 @@
+/**
+ * Copyright (c) 2012 - 2016 YCSB contributors. All rights reserved.


This is a new file?

risdenk · 2016-09-27T03:07:52Z

jdbc/src/main/java/com/yahoo/ycsb/db/flavors/DefaultDBFlavor.java

@@ -0,0 +1,98 @@
+/**
+ * Copyright (c) 2012 - 2016 YCSB contributors. All rights reserved.


This is a new file?

risdenk · 2016-09-27T03:07:59Z

jdbc/src/main/java/com/yahoo/ycsb/db/flavors/DBFlavor.java

@@ -0,0 +1,69 @@
+/**
+ * Copyright (c) 2012 - 2016 YCSB contributors. All rights reserved.


risdenk · 2016-09-27T03:08:07Z

jdbc/src/main/java/com/yahoo/ycsb/db/StatementType.java

@@ -0,0 +1,110 @@
+/**
+ * Copyright (c) 2010 - 2016 Yahoo! Inc. All rights reserved.


risdenk · 2016-09-27T03:09:11Z

core/src/main/java/com/yahoo/ycsb/DBWrapper.java

@@ -184,7 +184,7 @@ public Status scan(String table, String startkey, int recordcount,
  private void measure(String op, Status result, long intendedStartTimeNanos,
      long startTimeNanos, long endTimeNanos) {
    String measurementName = op;
-    if (result != Status.OK) {
+    if (result == null || !result.isOk()) {


Update copyright on changed file?

risdenk · 2016-09-27T03:09:30Z

core/src/main/java/com/yahoo/ycsb/Status.java

@@ -79,6 +79,14 @@ public boolean equals(Object obj) {
    return true;
  }

+  /**


Update copyright on changed file?

joshelser · 2016-09-27T03:18:58Z

Hi @risdenk! Thanks for taking a look -- it's greatly appreciated.

You're entirely right about the license headers: it was not clear to me what the copyright notice should be on new files. I just made a best guess on context. I am happy to fix with proper guidance.

risdenk · 2016-09-27T03:22:12Z

New files are usually:

Copyright (c) 2016 YCSB contributors. All rights reserved.

An example of one that got modified would be something like this:

Copyright (c) 2010 Yahoo! Inc., 2016 YCSB contributors. All rights reserved.

I'm just reciting what I've seen previously. Not sure this is set in stone. Maybe @busbey has an opinion?

busbey · 2016-09-27T15:18:49Z

yep, those suggested copyright statements look correct.

joshelser · 2016-09-27T17:24:03Z

yep, those suggested copyright statements look correct.

Great. Thanks @busbey and @risdenk! I'll fix these changes shortly with those recommendations.

Add Phoenix flavor for Apache Phoenix.

Tried to simplify the distinction between using JDBC's addBatch()/executeBatch() API calls and "batching" updates via autoCommit=false and a manual commit() after a given number of updates. Breaks out flavors into their own package to reduce bloat in JdbcDBClient. Encompasses changes from Enis Soztutar.

joshelser · 2016-09-28T17:18:30Z

Just updated things with more correct-er license headers. Thanks again for the close look, @risdenk!

risdenk

Changes look good to me now. Thanks @joshelser. I'll leave this open till Friday in case any other contributors have comments. If no comments and tests pass, I'll merge.

risdenk · 2016-09-30T13:02:22Z

@joshelser thanks for the PR!

joshelser · 2016-10-04T23:09:16Z

Thanks for the review and merge, @risdenk !

risdenk reviewed Sep 27, 2016

View reviewed changes

enis and others added 3 commits September 28, 2016 13:14

Add db.flavor to capture syntax differences among JDBC implementations.

74b5080

Add Phoenix flavor for Apache Phoenix.

[core] Treat BATCHED_OK as an acceptable status for insert()

4823a07

joshelser force-pushed the jdbc-fixes branch from 18d87ce to 4823a07 Compare September 28, 2016 17:16

risdenk approved these changes Sep 28, 2016

View reviewed changes

risdenk self-assigned this Sep 28, 2016

risdenk added bug jdbc improvement labels Sep 28, 2016

risdenk merged commit dfe0c64 into brianfrankcooper:master Sep 30, 2016

busbey mentioned this pull request Sep 30, 2016

turn on reviews? #855

Closed

risdenk mentioned this pull request Nov 15, 2016

Release version 0.12.0 #873

Closed

robert-s-lee mentioned this pull request Sep 26, 2020

support CockroachDB as of system time query #1460

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jdbc] Assorted cleanup/fixes around batching and supporting different databases #842

[jdbc] Assorted cleanup/fixes around batching and supporting different databases #842

joshelser commented Sep 20, 2016

joshelser commented Sep 20, 2016

joshelser commented Sep 20, 2016

risdenk left a comment

risdenk Sep 27, 2016

risdenk Sep 27, 2016

risdenk Sep 27, 2016

risdenk Sep 27, 2016

risdenk Sep 27, 2016

risdenk Sep 27, 2016

joshelser commented Sep 27, 2016

risdenk commented Sep 27, 2016

busbey commented Sep 27, 2016

joshelser commented Sep 27, 2016

joshelser commented Sep 28, 2016

risdenk left a comment

risdenk commented Sep 30, 2016

joshelser commented Oct 4, 2016

		@@ -0,0 +1,65 @@
		/**
		* Copyright (c) 2012 - 2016 YCSB contributors. All rights reserved.

		@@ -0,0 +1,98 @@
		/**
		* Copyright (c) 2012 - 2016 YCSB contributors. All rights reserved.

		@@ -0,0 +1,69 @@
		/**
		* Copyright (c) 2012 - 2016 YCSB contributors. All rights reserved.

		@@ -0,0 +1,110 @@
		/**
		* Copyright (c) 2010 - 2016 Yahoo! Inc. All rights reserved.

[jdbc] Assorted cleanup/fixes around batching and supporting different databases #842

[jdbc] Assorted cleanup/fixes around batching and supporting different databases #842

Conversation

joshelser commented Sep 20, 2016

joshelser commented Sep 20, 2016

joshelser commented Sep 20, 2016

risdenk left a comment

Choose a reason for hiding this comment

risdenk Sep 27, 2016

Choose a reason for hiding this comment

risdenk Sep 27, 2016

Choose a reason for hiding this comment

risdenk Sep 27, 2016

Choose a reason for hiding this comment

risdenk Sep 27, 2016

Choose a reason for hiding this comment

risdenk Sep 27, 2016

Choose a reason for hiding this comment

risdenk Sep 27, 2016

Choose a reason for hiding this comment

joshelser commented Sep 27, 2016

risdenk commented Sep 27, 2016

busbey commented Sep 27, 2016

joshelser commented Sep 27, 2016

joshelser commented Sep 28, 2016

risdenk left a comment

Choose a reason for hiding this comment

risdenk commented Sep 30, 2016

joshelser commented Oct 4, 2016