[rocksdb] Added support for RocksDB Java API #1052

adamretter · 2017-10-20T19:42:32Z

This adds support for RocksDB via RocksJava binding.

adamretter · 2017-11-29T14:14:28Z

@risdenk I have bought this up to date and in line with the changes you requested in #876

Closes #876

adamretter · 2017-12-12T21:56:01Z

@busbey Any chance of getting this one merged?

adamretter · 2018-02-09T16:23:42Z

@busbey @manolama can someone please take a look?

Vogel612

I really like that you added thorough Unit-Tests. Thanks for that :)

Vogel612 · 2018-04-02T16:05:44Z

rocksdb/README.md

+
+## Quick Start
+
+This section describes how to run YCSB on RocksDB running locally.


does it, though?

Yes I believe so. Why do you think otherwise?

I see no installation and setup instructions for RocksDB itself :/ As I understood it, these are also in the binding-related readme.md

You don't need to install it. Everything you need is self-contained inside the rocksdbjni Jar that YCSB declares a dependency on. Note that RocksDB is an embedded database, not a database server.

Vogel612 · 2018-04-02T16:06:01Z

rocksdb/pom.xml

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


it's 2018 by now

Vogel612 · 2018-04-02T16:06:16Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

@@ -0,0 +1,387 @@
+/*
+ * Copyright (c) 2017 YCSB contributors. All rights reserved.


Vogel612 · 2018-04-02T16:09:04Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+
+  @GuardedBy("RocksDBClient.class") private static Path rocksdbDir = null;
+  @GuardedBy("RocksDBClient.class") private static RocksObject dbOptions = null;
+  @GuardedBy("RocksDBClient.class") private static RocksDB rocksDb = null;


if this wasn't static, you'd not need all the synchronization.... What is the advantage of forcing the RocksDB object you're using to be a singleton?

I wanted to ensure that there was only a single instance of RocksDB running at any time within YCSB. Perhaps this is unnecessary?

I get that you want only a single instance of RocksDB. I don't understand why. For the general case, that is unnecessary, yes. It may be different for RocksDB though. I have never worked with it before...

So ... why do you want to ensure that there is only a single instance of RocksDB running at any time?
How do you propose dealing with multiple ycsb run invocations on the same host?

RocksDB is an embedded database (not a database server), i.e. it will be running inside the same JVM as YCSB itself. I wanted to ensure there was only one RocksDB instance per YCSB instance. Does that make sense?

Vogel612 · 2018-04-02T16:09:27Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+  public void init() throws DBException {
+    synchronized(RocksDBClient.class) {
+      if(rocksDb == null) {
+        rocksdbDir = Paths.get(getProperties().getProperty(PROP_ROCKSDB_DIR));


casing inconsistency: rocksDbDir

Vogel612 · 2018-04-02T16:18:43Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+
+      final ColumnFamilyHandle cf = COLUMN_FAMILIES.get(table).getHandle();
+      try(final RocksIterator iterator = rocksDb.newIterator(cf)) {
+        int iterations = 0;


you're never using the value stored in iterations. Why have that variable?

It is used on the line below - iterations < recordcount

oh right, I missed that. Please disregard that :)

Vogel612 · 2018-04-02T16:22:43Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+    final Path file = rocksdbDir.resolve(CF_NAMES_FILENAME);
+    if(Files.exists(file)) {
+      try (final LineNumberReader reader =
+               new LineNumberReader(new InputStreamReader(Files.newInputStream(file), UTF_8))) {


instead of new InputStreamReader(Files.newInputStream(file), UTF_8) you could use Files.newBufferedReader(file, UTF_8)

Vogel612 · 2018-04-02T16:23:53Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+
+  private void saveColumnFamilyNames() throws IOException {
+    final Path file = rocksdbDir.resolve(CF_NAMES_FILENAME);
+    try(final PrintWriter writer = new PrintWriter(new OutputStreamWriter(Files.newOutputStream(file), UTF_8))) {


instead of new OutputStreamWriter(Files.newOutputStream(file), UTF_8) you could use Files.newBufferedWriter(file, UTF_8)

Vogel612 · 2018-04-02T16:26:00Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/package-info.java

@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2017 YCSB contributors. All rights reserved.


Vogel612 · 2018-04-02T16:26:09Z

rocksdb/src/test/java/com/yahoo/ycsb/db/rocksdb/RocksDBClientTest.java

@@ -0,0 +1,121 @@
+/*
+ * Copyright (c) 2017 YCSB contributors. All rights reserved.


adamretter · 2018-04-02T17:32:16Z

@Vogel612 Thanks for the review. I have corrected everything as suggested (where possible).

The only outstanding question, is perhaps the static synchronization one. I would welcome a response to my comment on that, and then we can either fix, or leave as is...

busbey · 2017-12-03T01:17:45Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+ *
+ * See {@code rocksdb/README.md} for details.
+ *
+ * @author <a href="mailto:adam@evolvedbinary.com">Adam Retter</a>


We try to avoid @author lines. Instead we rely on git to show attribution.

Please leave this out.

Okay no problem, I have removed that now.

busbey · 2017-12-03T01:19:39Z

rocksdb/pom.xml

+    </dependency>
+    <dependency>
+      <groupId>net.jcip</groupId>
+      <artifactId>jcip-annotations</artifactId>


Just curious, is GuardedBy runtime scoped?

Nope, it is compile time.

adamretter · 2018-05-12T04:40:05Z

@busbey If you are happy with this, I can rebase to squash down the fixup commits before you merge. Just let me know...

busbey · 2018-05-19T14:27:28Z

A preemptive rebase+squash is always helpful, but I haven't had a chance to dig in yet. Planning to once 0.14.0 is wrapped.

adamretter · 2018-05-21T08:10:19Z

@busbey Sure. I have now squashed and rebased.

busbey

some questions and a couple of touch-ups.

busbey · 2018-06-14T02:57:07Z

rocksdb/README.md

+
+## Quick Start
+
+This section describes how to run YCSB on RocksDB running locally.


I think it's worth pointing out here that RocksDB is always embedded and so things like "How to run in parallel" don't make sense to do in this case.

busbey · 2018-06-14T03:00:56Z

rocksdb/pom.xml

+  <parent>
+    <groupId>com.yahoo.ycsb</groupId>
+    <artifactId>binding-parent</artifactId>
+    <version>0.14.0-SNAPSHOT</version>


please update this now that master is at 0.15.0-SNAPSHOT

busbey · 2018-06-14T03:10:55Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+    synchronized(RocksDBClient.class) {
+      if(rocksDb == null) {
+        rocksDbDir = Paths.get(getProperties().getProperty(PROPERTY_ROCKSDB_DIR));
+        System.out.println("RocksDB data dir: " + rocksDbDir);


please use a logging framework instead of System.out for messages. preferably slf4j.

busbey · 2018-06-14T03:15:12Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+    }
+  }
+
+  private RocksDB initRocksDB() throws IOException, RocksDBException {


since this has to be called within a lock on RocksDBClient.class, you should note that in javadoc on the method.

busbey · 2018-06-14T05:37:19Z

rocksdb/src/main/java/com/yahoo/ycsb/db/rocksdb/RocksDBClient.java

+    try {
+      if(!COLUMN_FAMILIES.containsKey(name)) {
+        final ColumnFamilyOptions cfOptions = new ColumnFamilyOptions().optimizeLevelStyleCompaction();
+        final ColumnFamilyHandle cfHandle = rocksDb.createColumnFamily(


I'm trying to work through the reasoning around locking. is this call to rocksDb.createColumnFamily the non-threadsafe thing we're using the column family lock to guard?

There are two different locking mechanisms at play here:

It seemed to me that there could be multiple RocksDBClient object instances within a running instance of YCSB. However, as RocksDB itself is an embedded database, we can only open and close it once for the entire lifecycle of YCSB. The synchronization on RocksDBClient.class ensures that RocksDB will only be opened once on the first call, and then only closed on the last call (ensured via reference counting).

A YCSB "table" is represented in RocksDB as a Column Family. We create these column families on demand, and we must only create them once (on their first use). The COLUMN_FAMILY_LOCKS are used to ensure that a column family is only created once no matter how many concurrent calls there are for an operation on the "table". Also note that, when you create a column family in RocksDB, the next time you open the database you have to tell it of all existing column families, otherwise it will error. This is why we also persist the column family names to a text file.

Hope that makes sense?

…base

busbey · 2018-07-07T04:04:29Z

thanks for getting this together @adamretter!

adamretter · 2018-07-07T05:04:57Z

@busbey Awesome thanks. We can now look at tuning the options and improving performance.

adamretter force-pushed the rocks-java branch 2 times, most recently from 39702ad to e1c285d Compare October 20, 2017 20:05

adamretter force-pushed the rocks-java branch 3 times, most recently from 65b98e3 to c0c6a93 Compare November 29, 2017 14:13

adamretter force-pushed the rocks-java branch from c0c6a93 to 7f7457b Compare April 2, 2018 11:42

adamretter mentioned this pull request Apr 2, 2018

PR for RocksDB being ignored :-( #1120

Closed

Vogel612 reviewed Apr 2, 2018

View reviewed changes

busbey requested changes May 11, 2018

View reviewed changes

busbey mentioned this pull request May 19, 2018

[rocksdb] RocksDB binding #876

Closed

busbey mentioned this pull request May 19, 2018

Add rocksdb-binding for ycsb. #1075

Closed

adamretter force-pushed the rocks-java branch from 2c2c4f7 to 764098a Compare May 21, 2018 08:09

busbey requested changes Jun 14, 2018

View reviewed changes

adamretter added 4 commits June 22, 2018 14:40

[rocksdb] Added support for RocksDB Java API

b6184eb

Added a note to README.md explaining that RocksDB is an embedded data…

2d78f8d

…base

Update to the latest YCSB binding parent

da12e56

Use SLF4j and console

050e338

adamretter force-pushed the rocks-java branch from 764098a to 050e338 Compare June 22, 2018 09:16

busbey approved these changes Jul 7, 2018

View reviewed changes

busbey changed the title ~~Added support for RocksDB~~ [rocksdb] Added support for RocksDB Java API Jul 7, 2018

busbey merged commit 563925e into brianfrankcooper:master Jul 7, 2018

adamretter deleted the rocks-java branch July 7, 2018 05:05

busbey mentioned this pull request Jul 29, 2018

Release 0.15.0 #1204

Closed


		## Quick Start

		This section describes how to run YCSB on RocksDB running locally.

		@@ -0,0 +1,387 @@
		/*
		* Copyright (c) 2017 YCSB contributors. All rights reserved.

		@@ -0,0 +1,22 @@
		/*
		* Copyright (c) 2017 YCSB contributors. All rights reserved.

		@@ -0,0 +1,121 @@
		/*
		* Copyright (c) 2017 YCSB contributors. All rights reserved.

[rocksdb] Added support for RocksDB Java API #1052

[rocksdb] Added support for RocksDB Java API #1052

Conversation

adamretter commented Oct 20, 2017

adamretter commented Nov 29, 2017

adamretter commented Dec 12, 2017

adamretter commented Feb 9, 2018

Vogel612 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamretter Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamretter commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamretter commented May 12, 2018

busbey commented May 19, 2018

adamretter commented May 21, 2018

busbey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamretter Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

busbey commented Jul 7, 2018

adamretter commented Jul 7, 2018

adamretter Apr 2, 2018 •

edited

Loading

adamretter Jun 22, 2018 •

edited

Loading