szetszwo commented on code in PR #3673:
URL: https://github.com/apache/ozone/pull/3673#discussion_r943125599
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/db/DatanodeDBProfile.java:
##########
@@ -120,15 +116,19 @@ private ManagedDBOptions getDBOptions() {
private ManagedColumnFamilyOptions getColumnFamilyOptions(
ConfigurationSource config) {
- cfOptions.updateAndGet(op -> op != null ? op :
- baseProfile.getColumnFamilyOptions()
- .setTableFormatConfig(getBlockBasedTableConfig(config)));
- return cfOptions.get();
+ ManagedColumnFamilyOptions cfOptions =
+ baseProfile.getColumnFamilyOptions();
+ if (cfOptions.tableFormatConfig() instanceof
+ ManagedBlockBasedTableConfig) {
+ ((ManagedBlockBasedTableConfig) cfOptions.tableFormatConfig()).close();
+ }
+ return cfOptions
+ .setTableFormatConfig(getBlockBasedTableConfig(config));
Review Comment:
There is a synchronization problem when multiple threads call this method so
that we need a closeAndSetTableFormatConfig method.
```java
//ManagedColumnFamilyOptions.java
public synchronized ManagedColumnFamilyOptions
closeAndSetTableFormatConfig(
TableFormatConfig tableFormatConfig) {
final TableFormatConfig previous = tableFormatConfig();
if (previous instanceof ManagedBlockBasedTableConfig) {
((ManagedBlockBasedTableConfig)previous).close();
}
setTableFormatConfig(tableFormatConfig);
return this;
}
```
Then, this method becomes
```java
private ManagedColumnFamilyOptions getColumnFamilyOptions(
ConfigurationSource config) {
ManagedColumnFamilyOptions cfOptions =
baseProfile.getColumnFamilyOptions();
return cfOptions.closeAndSetTableFormatConfig(
getBlockBasedTableConfig(config));
}
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedColumnFamilyOptions.java:
##########
@@ -36,6 +36,16 @@ public ManagedColumnFamilyOptions(ColumnFamilyOptions
columnFamilyOptions) {
@Override
public ManagedColumnFamilyOptions setTableFormatConfig(
Review Comment:
This should be synchronized.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -405,7 +400,7 @@ public ColumnFamily getColumnFamily(String key) {
return columnFamilies.get(key);
}
- public Collection<ColumnFamily> getColumnFamilies() {
+ public Collection<ColumnFamily> getExtraColumnFamilies() {
Review Comment:
IDE error? This method should not be renamed.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStoreBuilder.java:
##########
@@ -274,6 +274,13 @@ private Set<TableConfig> makeTableConfigs() {
return tableConfigs;
}
+ private ManagedColumnFamilyOptions getDefaultCfOptions() {
+ if (defaultCfOptions != null) {
+ return defaultCfOptions;
+ }
+ return defaultCfProfile.getColumnFamilyOptions();
Review Comment:
FYI, we may use Optional
```java
return Optional.ofNullable(defaultCfOptions)
.orElseGet(defaultCfProfile::getColumnFamilyOptions);
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/db/DatanodeDBProfile.java:
##########
@@ -138,6 +138,9 @@ private BlockBasedTableConfig getBlockBasedTableConfig(
.getStorageSize(HDDS_DATANODE_METADATA_ROCKSDB_CACHE_SIZE,
HDDS_DATANODE_METADATA_ROCKSDB_CACHE_SIZE_DEFAULT,
StorageUnit.BYTES);
+ if (blockBasedTableConfig.blockCache() != null) {
+ blockBasedTableConfig.blockCache().close();
+ }
Review Comment:
Similarly, we need a `closeAndSetBlockCache` in
`ManagedBlockBasedTableConfig`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
