jojochuang commented on code in PR #11671:
URL: https://github.com/apache/ozone/pull/11671#discussion_r4345967217
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDBConfiguration.java:
##########
@@ -84,6 +84,20 @@ public class RocksDBConfiguration {
+ "Default 0 means no limit.")
private long walSizeLimit = 0;
+ @Config(key = "hadoop.hdds.db.rocksdb.compaction.deletion.ratio",
+ type = ConfigType.DOUBLE,
+ defaultValue = "0",
+ tags = {OM, SCM, DATANODE},
Review Comment:
Good catch. Addressed in
[c2c969773ee](https://github.com/jojochuang/ozone/commit/c2c969773ee0c0a6fc102187aa99519ca9104440):
the deletion ratio now goes through
`DatanodeDBProfile.StorageBasedProfile#createColumnFamilyOptions`, so both
schema V2 and V3 container DBs receive it under SSD/DISK profiles. Added four
native-compaction cases covering both profiles and schemas; they also verify
deleted keys stay absent and all 250 surviving entries remain intact.
Options-file overrides retain precedence. I updated the PR description to
reflect the coverage and explicitly state that schema V3 container deletion via
`deleteRange` is not counted by this collector.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestDBStoreBuilder.java:
##########
@@ -82,6 +84,80 @@ public void builderWithOpenClose(@TempDir Path tempDir)
throws Exception {
dbStore.close();
}
+ @ParameterizedTest
+ @EnumSource(value = DBProfile.class, names = {"SSD", "DISK"})
+ public void testCompactionOnDeletion(DBProfile profile, @TempDir Path
tempDir) throws Exception {
+ OzoneConfiguration conf = new OzoneConfiguration();
+ conf.setDouble("hadoop.hdds.db.rocksdb.compaction.deletion.ratio", 0.5);
+ conf.setEnum(HddsConfigKeys.HDDS_DB_PROFILE, profile);
+ try (RDBStore store =
DBStoreBuilder.newBuilder(conf).setName("deletions.db")
+ .setPath(tempDir).addTable("test").build()) {
+ Table<byte[], byte[]> table = store.getTable("test");
+ for (int i = 0; i < 1000; i++) {
+ table.put(StringCodec.get().toPersistedFormat(String.format("%04d",
i)), new byte[100]);
+ }
+ store.flushDB();
+ for (int i = 0; i < 750; i++) {
+ table.delete(StringCodec.get().toPersistedFormat(String.format("%04d",
i)));
+ }
+ store.flushDB();
+ GenericTestUtils.waitFor(() -> {
+ try {
+ return store.getDb().getManagedRocksDb().get()
+
.getPropertiesOfAllTables(store.getDb().getColumnFamily("test").getHandle()).values().stream()
+ .mapToLong(properties -> properties.getNumDeletions()).sum() ==
0;
+ } catch (Exception e) {
+ throw new IllegalStateException(e);
+ }
+ }, 100, 10000);
+ int count = 0;
+ try (Table.KeyValueIterator<byte[], byte[]> iterator = table.iterator())
{
+ while (iterator.hasNext()) {
+ Table.KeyValue<byte[], byte[]> entry = iterator.next();
+
assertArrayEquals(StringCodec.get().toPersistedFormat(String.format("%04d", 750
+ count)), entry.getKey());
+ assertArrayEquals(new byte[100], entry.getValue());
+ count++;
+ }
+ }
+ assertEquals(250, count);
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(doubles = {0, 1})
+ public void testCompactionNotTriggered(double ratio, @TempDir Path tempDir)
throws Exception {
+ OzoneConfiguration conf = new OzoneConfiguration();
+ if (ratio != 0) {
+ conf.setDouble("hadoop.hdds.db.rocksdb.compaction.deletion.ratio",
ratio);
+ }
+ try (RDBStore store =
DBStoreBuilder.newBuilder(conf).setName("deletions.db")
+ .setPath(tempDir).addTable("test").setProfile(DBProfile.DISK).build())
{
+ Table<byte[], byte[]> table = store.getTable("test");
+ for (int i = 0; i < 1000; i++) {
+ table.put(StringCodec.get().toPersistedFormat(String.format("%04d",
i)), new byte[100]);
+ }
+ store.flushDB();
+ for (int i = 0; i < 750; i++) {
+ table.delete(StringCodec.get().toPersistedFormat(String.format("%04d",
i)));
+ }
+ // Keep the second SST's deletion ratio below 1.
+ table.put(StringCodec.get().toPersistedFormat("new-key"), new byte[100]);
+ store.flushDB();
+ assertEquals(750, store.getDb().getManagedRocksDb().get()
+
.getPropertiesOfAllTables(store.getDb().getColumnFamily("test").getHandle()).values().stream()
+ .mapToLong(properties -> properties.getNumDeletions()).sum());
Review Comment:
Agreed—the immediate assertion could pass before background compaction ran.
Fixed in
[c2c969773ee](https://github.com/jojochuang/ozone/commit/c2c969773ee0c0a6fc102187aa99519ca9104440)
using the suggested two-second bounded wait for the deletion count to drop
below 750. I also temporarily added `0.5` as a negative control: that case
failed as expected in about 200 ms, while `0` and `1` passed. Restored the
intended parameters afterward. The final focused run passed all 30 tests across
managed RocksDB, framework, and datanode coverage.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestDBStoreBuilder.java:
##########
@@ -82,6 +84,80 @@ public void builderWithOpenClose(@TempDir Path tempDir)
throws Exception {
dbStore.close();
}
+ @ParameterizedTest
+ @EnumSource(value = DBProfile.class, names = {"SSD", "DISK"})
+ public void testCompactionOnDeletion(DBProfile profile, @TempDir Path
tempDir) throws Exception {
+ OzoneConfiguration conf = new OzoneConfiguration();
+ conf.setDouble("hadoop.hdds.db.rocksdb.compaction.deletion.ratio", 0.5);
+ conf.setEnum(HddsConfigKeys.HDDS_DB_PROFILE, profile);
+ try (RDBStore store =
DBStoreBuilder.newBuilder(conf).setName("deletions.db")
+ .setPath(tempDir).addTable("test").build()) {
+ Table<byte[], byte[]> table = store.getTable("test");
+ for (int i = 0; i < 1000; i++) {
Review Comment:
Extracted `writeThenDelete` and `numDeletions` in
[c2c969773ee](https://github.com/jojochuang/ozone/commit/c2c969773ee0c0a6fc102187aa99519ca9104440),
and reused them in both compaction tests. The workload and verification of the
remaining keys are preserved.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDBConfiguration.java:
##########
@@ -84,6 +84,20 @@ public class RocksDBConfiguration {
+ "Default 0 means no limit.")
private long walSizeLimit = 0;
+ @Config(key = "hadoop.hdds.db.rocksdb.compaction.deletion.ratio",
+ type = ConfigType.DOUBLE,
+ defaultValue = "0",
+ tags = {OM, SCM, DATANODE},
+ description = "Request automatic compaction when the fraction of
deletion tombstones in a newly created SST "
+ + "reaches this ratio. Valid values are between 0 and 1; 0 disables
the collector. "
+ + "Applies to default DB profiles; explicit column family options
and options files take precedence. "
+ + "Does not flush tombstones from memtables or override disabled
automatic compaction. Requires restart.")
+ private double compactionDeletionRatio;
Review Comment:
Added `@PostConstruct` validation in `RocksDBConfiguration` in
[c2c969773ee](https://github.com/jojochuang/ozone/commit/c2c969773ee0c0a6fc102187aa99519ca9104440).
The error now includes `hadoop.hdds.db.rocksdb.compaction.deletion.ratio`, and
the invalid-value tests assert that property name. The bounds check also
rejects NaN and infinity. I retained the lower-level validation in
`ManagedColumnFamilyOptions.withDeletionRatio` for direct callers.
--
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]
