Myasuka commented on code in PR #22458:
URL: https://github.com/apache/flink/pull/22458#discussion_r1186599166
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBResourceContainer.java:
##########
@@ -82,30 +89,36 @@ public final class RocksDBResourceContainer implements
AutoCloseable {
@VisibleForTesting
public RocksDBResourceContainer() {
- this(new Configuration(), PredefinedOptions.DEFAULT, null, null,
false);
+ this(null, new Configuration(), PredefinedOptions.DEFAULT, null, null,
false);
Review Comment:
I feel it really strange that we put the `null` in the first place, can we
put the nullable parameter in the last places.
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBResourceContainer.java:
##########
@@ -82,30 +89,36 @@ public final class RocksDBResourceContainer implements
AutoCloseable {
@VisibleForTesting
public RocksDBResourceContainer() {
- this(new Configuration(), PredefinedOptions.DEFAULT, null, null,
false);
+ this(null, new Configuration(), PredefinedOptions.DEFAULT, null, null,
false);
}
@VisibleForTesting
public RocksDBResourceContainer(
PredefinedOptions predefinedOptions, @Nullable
RocksDBOptionsFactory optionsFactory) {
- this(new Configuration(), predefinedOptions, optionsFactory, null,
false);
+ this(null, new Configuration(), predefinedOptions, optionsFactory,
null, false);
}
@VisibleForTesting
public RocksDBResourceContainer(
PredefinedOptions predefinedOptions,
@Nullable RocksDBOptionsFactory optionsFactory,
@Nullable OpaqueMemoryResource<RocksDBSharedResources>
sharedResources) {
- this(new Configuration(), predefinedOptions, optionsFactory,
sharedResources, false);
+ this(null, new Configuration(), predefinedOptions, optionsFactory,
sharedResources, false);
}
public RocksDBResourceContainer(
+ @Nullable File instanceBasePath,
ReadableConfig configuration,
PredefinedOptions predefinedOptions,
@Nullable RocksDBOptionsFactory optionsFactory,
@Nullable OpaqueMemoryResource<RocksDBSharedResources>
sharedResources,
boolean enableStatistics) {
+ this.instanceBasePath = instanceBasePath;
+ this.instanceRocksDBPath =
+ instanceBasePath != null
+ ? new File(instanceBasePath, DB_INSTANCE_DIR_STRING)
Review Comment:
I think the true place to initialize the instance rocksdb path is in
[RocksDBKeyedStateBackendBuilder](https://github.com/apache/flink/blob/3379c2c085da43bb452536c981a7fc13f39482ee/flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackendBuilder.java#L169),
I prefer we can use same shared static method here to align with the actual
logic.
--
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]
