Copilot commented on code in PR #11671:
URL: https://github.com/apache/ozone/pull/11671#discussion_r4342458911
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestDBStoreBuilder.java:
##########
@@ -82,6 +84,71 @@ 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);
+ assertEquals(250, table.getEstimatedKeyCount());
Review Comment:
This test is likely to be flaky: (1) throwing from the `waitFor` supplier
fails immediately rather than continuing to wait; returning `false` on
transient exceptions is more robust, (2) `getEstimatedKeyCount()` is an
estimate and may not be exactly 250 immediately after background compaction.
Consider making the wait loop resilient to transient errors and validating key
count deterministically (e.g., by iterating/counting actual keys, or allowing a
tolerance/range if using an estimate). Also consider a larger timeout to
accommodate slower CI hosts.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedColumnFamilyOptions.java:
##########
@@ -42,6 +45,27 @@ public ManagedColumnFamilyOptions(ColumnFamilyOptions
columnFamilyOptions) {
super(columnFamilyOptions);
}
+ private ManagedColumnFamilyOptions(Options options) {
+ super(options);
+ }
+
+ /** Creates options that request compaction when an SST's deletion ratio
reaches the given threshold. */
+ public static ManagedColumnFamilyOptions withDeletionRatio(double
deletionRatio) {
+ if (!Double.isFinite(deletionRatio) || deletionRatio < 0 || deletionRatio
> 1) {
+ throw new IllegalArgumentException("Deletion ratio must be between 0 and
1: " + deletionRatio);
+ }
+ if (deletionRatio == 0) {
+ return new ManagedColumnFamilyOptions();
+ }
+ // RocksDB exposes the collector setter only on Options. The native
options retain a shared pointer to the factory.
+ try (ManagedOptions options = new ManagedOptions();
+ TablePropertiesCollectorFactory factory =
+
TablePropertiesCollectorFactory.NewCompactOnDeletionCollectorFactory(0, 0,
deletionRatio)) {
+
options.setTablePropertiesCollectorFactory(Collections.singletonList(factory));
+ return new ManagedColumnFamilyOptions(options);
+ }
Review Comment:
Potential lifetime issue: `factory` is closed at the end of the
try-with-resources, but the configured collector must remain valid for the
resulting `ColumnFamilyOptions` / RocksDB instance. If the RocksDB Java binding
does not deep-copy/retain the factory, this can cause use-after-free or
silently disable collection. Safer approach: do not close the
`TablePropertiesCollectorFactory` immediately; instead, attach it to the
managed lifecycle (e.g., via existing tracking/ownership mechanisms) so it is
closed when the `ManagedColumnFamilyOptions` is closed, after RocksDB no longer
needs it.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestDBStoreBuilder.java:
##########
@@ -82,6 +84,71 @@ 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);
+ assertEquals(250, table.getEstimatedKeyCount());
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(doubles = {0, 1})
+ public void testDeletionRatioBelowThreshold(double ratio, @TempDir Path
tempDir) throws Exception {
Review Comment:
Test name is misleading: the parameters include `ratio = 1`, which is a
valid threshold value (not strictly 'below threshold'). Consider renaming to
reflect the intent (e.g., 'does not trigger compaction for disabled collector
or when SST deletion ratio is below configured threshold'), or adjusting inputs
so they match the method name.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedColumnFamilyOptions.java:
##########
@@ -42,6 +45,27 @@ public ManagedColumnFamilyOptions(ColumnFamilyOptions
columnFamilyOptions) {
super(columnFamilyOptions);
}
+ private ManagedColumnFamilyOptions(Options options) {
+ super(options);
+ }
+
+ /** Creates options that request compaction when an SST's deletion ratio
reaches the given threshold. */
+ public static ManagedColumnFamilyOptions withDeletionRatio(double
deletionRatio) {
+ if (!Double.isFinite(deletionRatio) || deletionRatio < 0 || deletionRatio
> 1) {
+ throw new IllegalArgumentException("Deletion ratio must be between 0 and
1: " + deletionRatio);
Review Comment:
The validation allows `0` and `1` (inclusive), but the error text 'between 0
and 1' can be read as exclusive. Consider clarifying the range in the message
(e.g., 'between 0 and 1 inclusive') to reduce confusion when operators
misconfigure the value.
--
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]
