Skip to content

Commit

Permalink
WriteBufferManager JNI fixes (#4579)
Browse files Browse the repository at this point in the history
Summary:
1. `WriteBufferManager` should have a reference alive in Java side through `Options`/`DBOptions` otherwise, if it's GC'ed at java side, native side can seg fault.
2. native method `setWriteBufferManager()` in `DBOptions.java` doesn't have it's jni method invocation in rocksdbjni which is added in this PR
3. `DBOptionsTest.java` is referencing object of `Options`. Instead it should be testing against `DBOptions`. Seems like a copy paste error.
4. Add a getter for WriteBufferManager.
Pull Request resolved: #4579

Differential Revision: D10561150

Pulled By: sagar0

fbshipit-source-id: 139a15c7f051a9f77b4200215b88267b48fbc487
  • Loading branch information
jbhati authored and facebook-github-bot committed Oct 24, 2018
1 parent 8c78348 commit 6ecd26a
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 2 deletions.
14 changes: 14 additions & 0 deletions java/rocksjni/options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5532,6 +5532,20 @@ void Java_org_rocksdb_DBOptions_setDbWriteBufferSize(
opt->db_write_buffer_size = static_cast<size_t>(jdb_write_buffer_size);
}

/*
* Class: org_rocksdb_DBOptions
* Method: setWriteBufferManager
* Signature: (JJ)V
*/
void Java_org_rocksdb_DBOptions_setWriteBufferManager(JNIEnv* /*env*/, jobject /*jobj*/,
jlong jdb_options_handle,
jlong jwrite_buffer_manager_handle) {
auto* write_buffer_manager =
reinterpret_cast<std::shared_ptr<rocksdb::WriteBufferManager> *>(jwrite_buffer_manager_handle);
reinterpret_cast<rocksdb::DBOptions*>(jdb_options_handle)->write_buffer_manager =
*write_buffer_manager;
}

/*
* Class: org_rocksdb_DBOptions
* Method: dbWriteBufferSize
Expand Down
9 changes: 9 additions & 0 deletions java/src/main/java/org/rocksdb/DBOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public DBOptions(DBOptions other) {
this.numShardBits_ = other.numShardBits_;
this.rateLimiter_ = other.rateLimiter_;
this.rowCache_ = other.rowCache_;
this.writeBufferManager_ = other.writeBufferManager_;
}

/**
Expand Down Expand Up @@ -671,10 +672,17 @@ public DBOptions setDbWriteBufferSize(final long dbWriteBufferSize) {
public DBOptions setWriteBufferManager(final WriteBufferManager writeBufferManager) {
assert(isOwningHandle());
setWriteBufferManager(nativeHandle_, writeBufferManager.nativeHandle_);
this.writeBufferManager_ = writeBufferManager;
return this;
}

@Override
public WriteBufferManager writeBufferManager() {
assert(isOwningHandle());
return this.writeBufferManager_;
}

@Override
public long dbWriteBufferSize() {
assert(isOwningHandle());
return dbWriteBufferSize(nativeHandle_);
Expand Down Expand Up @@ -1167,4 +1175,5 @@ private native void setAvoidFlushDuringShutdown(final long handle,
private int numShardBits_;
private RateLimiter rateLimiter_;
private Cache rowCache_;
private WriteBufferManager writeBufferManager_;
}
9 changes: 9 additions & 0 deletions java/src/main/java/org/rocksdb/DBOptionsInterface.java
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,15 @@ public interface DBOptionsInterface<T extends DBOptionsInterface> {
*/
T setWriteBufferManager(final WriteBufferManager writeBufferManager);

/**
* Reference to {@link WriteBufferManager} used by it. <br>
*
* Default: null (Disabled)
*
* @return a reference to WriteBufferManager
*/
WriteBufferManager writeBufferManager();

/**
* Amount of data to build up in memtables across all column
* families before writing to disk.
Expand Down
9 changes: 9 additions & 0 deletions java/src/main/java/org/rocksdb/Options.java
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public Options(Options other) {
this.compactionOptionsFIFO_ = other.compactionOptionsFIFO_;
this.compressionOptions_ = other.compressionOptions_;
this.rowCache_ = other.rowCache_;
this.writeBufferManager_ = other.writeBufferManager_;
}

@Override
Expand Down Expand Up @@ -727,10 +728,17 @@ public Options setDbWriteBufferSize(final long dbWriteBufferSize) {
public Options setWriteBufferManager(final WriteBufferManager writeBufferManager) {
assert(isOwningHandle());
setWriteBufferManager(nativeHandle_, writeBufferManager.nativeHandle_);
this.writeBufferManager_ = writeBufferManager;
return this;
}

@Override
public WriteBufferManager writeBufferManager() {
assert(isOwningHandle());
return this.writeBufferManager_;
}

@Override
public long dbWriteBufferSize() {
assert(isOwningHandle());
return dbWriteBufferSize(nativeHandle_);
Expand Down Expand Up @@ -1918,4 +1926,5 @@ private native void setForceConsistencyChecks(final long handle,
private CompactionOptionsFIFO compactionOptionsFIFO_;
private CompressionOptions compressionOptions_;
private Cache rowCache_;
private WriteBufferManager writeBufferManager_;
}
6 changes: 4 additions & 2 deletions java/src/test/java/org/rocksdb/DBOptionsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -426,19 +426,21 @@ public void dbWriteBufferSize() {

@Test
public void setWriteBufferManager() throws RocksDBException {
try (final Options opt = new Options();
try (final DBOptions opt = new DBOptions();
final Cache cache = new LRUCache(1 * 1024 * 1024);
final WriteBufferManager writeBufferManager = new WriteBufferManager(2000l, cache)) {
opt.setWriteBufferManager(writeBufferManager);
assertThat(opt.writeBufferManager()).isEqualTo(writeBufferManager);
}
}

@Test
public void setWriteBufferManagerWithZeroBufferSize() throws RocksDBException {
try (final Options opt = new Options();
try (final DBOptions opt = new DBOptions();
final Cache cache = new LRUCache(1 * 1024 * 1024);
final WriteBufferManager writeBufferManager = new WriteBufferManager(0l, cache)) {
opt.setWriteBufferManager(writeBufferManager);
assertThat(opt.writeBufferManager()).isEqualTo(writeBufferManager);
}
}

Expand Down
2 changes: 2 additions & 0 deletions java/src/test/java/org/rocksdb/OptionsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,7 @@ public void setWriteBufferManager() throws RocksDBException {
final Cache cache = new LRUCache(1 * 1024 * 1024);
final WriteBufferManager writeBufferManager = new WriteBufferManager(2000l, cache)) {
opt.setWriteBufferManager(writeBufferManager);
assertThat(opt.writeBufferManager()).isEqualTo(writeBufferManager);
}
}

Expand All @@ -660,6 +661,7 @@ public void setWriteBufferManagerWithZeroBufferSize() throws RocksDBException {
final Cache cache = new LRUCache(1 * 1024 * 1024);
final WriteBufferManager writeBufferManager = new WriteBufferManager(0l, cache)) {
opt.setWriteBufferManager(writeBufferManager);
assertThat(opt.writeBufferManager()).isEqualTo(writeBufferManager);
}
}

Expand Down

0 comments on commit 6ecd26a

Please sign in to comment.