pnowojski commented on code in PR #22771:
URL: https://github.com/apache/flink/pull/22771#discussion_r1228225553
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBWriteBatchWrapper.java:
##########
@@ -79,7 +82,16 @@ public RocksDBWriteBatchWrapper(
Preconditions.checkArgument(batchSize >= 0, "Max batch size have to be
no negative.");
this.db = rocksDB;
- this.options = options;
+ if (options != null) {
+ this.options = options;
+ // ensure we don't close the object we don't own
+ this.ownsWriteOptions = false;
+ } else {
+ // Use default write options with disabled WAL
+ this.options = new WriteOptions().setDisableWAL(true);
+ // ensure we close the object we own
+ this.ownsWriteOptions = true;
+ }
Review Comment:
Shouldn't the fix be that we make sure options are always passed instead of
creating dummy options? 🤔
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBWriteBatchWrapper.java:
##########
@@ -125,10 +130,16 @@ public WriteOptions getOptions() {
@Override
public void close() throws RocksDBException {
- if (batch.count() != 0) {
- flush();
+ try {
+ if (batch.count() != 0) {
+ flush();
+ }
+ } finally {
+ IOUtils.closeQuietly(batch);
+ if (ownsWriteOptions) {
+ IOUtils.closeQuietly(options);
+ }
Review Comment:
nitty nit: use closer instead of closing each individually? That would also
eliminate this extra if check here, as in the constructor we could simply
add/or don't add options to the closer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
