smengcl commented on code in PR #11671:
URL: https://github.com/apache/ozone/pull/11671#discussion_r4342815081
##########
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:
This runs right after the flush, before compaction can finish, so it passes
even when compaction is triggered.
Adding `0.5` to the `@ValueSource` still passes locally. With a bounded
wait, `0.5` fails in ~230 ms and `0`/`1` pass:
```suggestion
assertThrows(TimeoutException.class, () -> GenericTestUtils.waitFor(()
-> {
try {
return store.getDb().getManagedRocksDb().get()
.getPropertiesOfAllTables(store.getDb().getColumnFamily("test").getHandle()).values().stream()
.mapToLong(TableProperties::getNumDeletions).sum() < 750;
} catch (Exception e) {
throw new IllegalStateException(e);
}
}, 100, 2000));
```
Needs imports for `TimeoutException` and `TableProperties`.
##########
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:
nit: both tests repeat the setup and the deletion count. Extract helpers:
```diff
+ private static void writeThenDelete(RDBStore store, Table<byte[], byte[]>
table) throws Exception {
+ 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)));
+ }
+ }
+
+ private static long numDeletions(RDBStore store) throws Exception {
+ return store.getDb().getManagedRocksDb().get()
+
.getPropertiesOfAllTables(store.getDb().getColumnFamily("test").getHandle()).values().stream()
+ .mapToLong(TableProperties::getNumDeletions).sum();
+ }
```
##########
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:
nit: an invalid value fails startup with `Deletion ratio must be between 0
and 1 inclusive: 1.1`, without the key name. Validate here, like
`OzoneClientConfig#validate`:
```diff
+ @PostConstruct
+ public void validate() {
+ Preconditions.checkArgument(compactionDeletionRatio >= 0 &&
compactionDeletionRatio <= 1,
+ "hadoop.hdds.db.rocksdb.compaction.deletion.ratio must be between 0
and 1 inclusive: %s", compactionDeletionRatio);
+ }
```
`testInvalidDeletionRatio` still passes.
##########
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:
Datanode container DBs (schema V2/V3) don't get this.
`DatanodeDBProfile.StorageBasedProfile#createColumnFamilyOptions` calls
`baseProfile.getColumnFamilyOptions()`, so the ratio is 0, and
`AbstractRDBStore` passes those options explicitly. On a datanode only
`datanode.db` gets the collector, not the container DB where block deletes
happen.
```diff
- ManagedColumnFamilyOptions options =
- baseProfile.getColumnFamilyOptions();
+ double deletionRatio = config == null ? 0 :
config.getObject(RocksDBConfiguration.class).getCompactionDeletionRatio();
+ ManagedColumnFamilyOptions options =
baseProfile.getColumnFamilyOptions(deletionRatio);
```
If this is intentional, pls say so in the description. Also, the collector
doesn't count the `deleteRange` tombstones from schema V3 container deletion.
--
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]
