adoroszlai commented on code in PR #8173:
URL: https://github.com/apache/ozone/pull/8173#discussion_r2077796740
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -2703,6 +2703,14 @@
Absolute path to HDDS metadata dir.
</description>
</property>
+ <property>
+ <name>datanode.db.config.path</name>
Review Comment:
Ozone Datanode config keys are usually prefixed with `hdds.datanode`, not
just `datanode`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java:
##########
@@ -110,17 +118,14 @@ public DatanodeSchemaThreeDBDefinition(String dbPath,
DatanodeDBProfile dbProfile = DatanodeDBProfile
.getProfile(config.getEnum(HDDS_DB_PROFILE, HDDS_DEFAULT_DB_PROFILE));
- ManagedColumnFamilyOptions cfOptions =
- dbProfile.getColumnFamilyOptions(config);
- // Use prefix seek to mitigating seek overhead.
- // See: https://github.com/facebook/rocksdb/wiki/Prefix-Seek
- cfOptions.useFixedLengthPrefixExtractor(getContainerKeyPrefixLength());
+ Path optionsPath = Paths.get(
+ config.get(HddsConfigKeys.DATANODE_DB_CONFIG_PATH,
HddsConfigKeys.DATANODE_DB_CONFIG_PATH_DEFAULT));
- BLOCK_DATA.setCfOptions(cfOptions);
- METADATA.setCfOptions(cfOptions);
- DELETE_TRANSACTION.setCfOptions(cfOptions);
- FINALIZE_BLOCKS.setCfOptions(cfOptions);
- LAST_CHUNK_INFO.setCfOptions(cfOptions);
+ BLOCK_DATA.setCfOptions(getCFOptions(config, dbProfile, optionsPath,
"block_data"));
+ METADATA.setCfOptions(getCFOptions(config, dbProfile, optionsPath,
"metadata"));
+ DELETE_TRANSACTION.setCfOptions(getCFOptions(config, dbProfile,
optionsPath, "delete_txns"));
+ FINALIZE_BLOCKS.setCfOptions(getCFOptions(config, dbProfile, optionsPath,
"finalize_blocks"));
+ LAST_CHUNK_INFO.setCfOptions(getCFOptions(config, dbProfile, optionsPath,
"last_chunk_info"));
Review Comment:
These were supposed to use the new constants for last argument.
But the name can be accessed like: `BLOCK_DATA.getName()`, so extracting the
constants is not even necessary.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java:
##########
@@ -191,4 +196,13 @@ public static long getContainerId(String key) {
private void setSeparator(String keySeparator) {
separator = keySeparator;
}
+
+ private ManagedColumnFamilyOptions getCFOptions(
+ ConfigurationSource config, DatanodeDBProfile dbProfile, Path
pathToOptions, String cfName) {
+ // Use prefix seek to mitigating seek overhead.
+ // See: https://github.com/facebook/rocksdb/wiki/Prefix-Seek
+ return (ManagedColumnFamilyOptions) dbProfile
+ .getColumnFamilyOptions(config, pathToOptions, cfName)
+ .useFixedLengthPrefixExtractor(getContainerKeyPrefixLength());
+ }
Review Comment:
Suggestion for improvement:
Pass the `DBColumnFamilyDefinition` instead of `cfName`, get column family
name from `DBColumnFamilyDefinition`, and `setCfOptions` on it instead of
returning. This reduces the possibility of mixing up
`DBColumnFamilyDefinition` and name.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStoreBuilder.java:
##########
@@ -410,38 +372,22 @@ private ManagedDBOptions getDefaultDBOptions(
}
/**
- * Attempts to construct a {@link ManagedDBOptions} object from the
- * configuration directory with name equal to {@code database name}.ini,
- * where {@code database name} is the property set by
- * {@link DBStoreBuilder#setName(String)}.
+ * Attempts to get RocksDB {@link ManagedColumnFamilyOptions} from an ini
config
+ * file. If that file does not exist, the value of {@code
getColumnFamilyOptions}
+ * is used instead.
+ *
+ * @return The {@link ManagedColumnFamilyOptions} that should be used as the
default
+ * value for this builder if one is not specified by the caller.
*/
- private ManagedDBOptions getDBOptionsFromFile(
- Collection<TableConfig> tableConfigs) {
- ManagedDBOptions option = null;
-
- List<ColumnFamilyDescriptor> columnFamilyDescriptors = new ArrayList<>();
-
- if (StringUtil.isNotBlank(dbname)) {
- for (TableConfig tc : tableConfigs) {
- columnFamilyDescriptors.add(tc.getDescriptor());
- }
-
- if (!columnFamilyDescriptors.isEmpty()) {
- try {
- option = DBConfigFromFile.readFromFile(dbname,
- columnFamilyDescriptors);
- if (option != null) {
- LOG.info("Using RocksDB DBOptions from {}.ini file", dbname);
- }
- } catch (IOException ex) {
- LOG.info("Unable to read RocksDB DBOptions from {}", dbname, ex);
- } finally {
- columnFamilyDescriptors.forEach(d -> d.getOptions().close());
- }
- }
+ private ManagedColumnFamilyOptions getCfOptions(String cfName) {
+ if (Objects.nonNull(defaultCfOptions)) {
+ return defaultCfOptions;
}
-
- return option;
+ if (Objects.isNull(defaultCfProfile)) {
+ throw new RuntimeException();
+ }
Review Comment:
Use `Objects.requireNonNull` with a short message to indicate what is
`null`. Example:
https://github.com/apache/ozone/blob/49b8fbd8905601b417f8fd5e12ef6997b72108d3/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/client/io/SelectorOutputStream.java#L56
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -2703,6 +2703,14 @@
Absolute path to HDDS metadata dir.
</description>
</property>
+ <property>
+ <name>datanode.db.config.path</name>
+ <value/>
+ <tag>OZONE, CONTAINER, STORAGE</tag>
+ <description>
+ Path to an ini configuration file for RocksDB on datanode compontent.
Review Comment:
```suggestion
Path to an ini configuration file for RocksDB on datanode component.
```
--
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]
