jojochuang commented on code in PR #11671:
URL: https://github.com/apache/ozone/pull/11671#discussion_r4342562228
##########
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:
Checked the RocksDB 10.10.1 JNI implementation: the setter copies the
factory's shared_ptr into Options, and the ColumnFamilyOptions constructor
copies those native options. Closing the Java factory wrapper only releases its
own reference; the returned column family options retain ownership. Keeping the
existing try-with-resources lifecycle avoids retaining an unnecessary wrapper.
The native compaction tests also open and use the DB after these temporary
resources have closed.
Setter:
https://github.com/facebook/rocksdb/blob/v10.10.1/java/rocksjni/options.cc#L3902-L3922
ColumnFamilyOptions copy:
https://github.com/facebook/rocksdb/blob/v10.10.1/java/rocksjni/options.cc#L3954-L3959
Wrapper disposal:
https://github.com/facebook/rocksdb/blob/v10.10.1/java/rocksjni/table_properties_collector_factory.cc#L34-L39
##########
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:
Addressed the estimated-count concern in dc82d36d541: the test now iterates
the actual entries, checks every surviving key and value, and asserts the exact
count. Unexpected RocksDB exceptions still fail immediately so genuine errors
are not hidden by retries. The existing timeout is retained; there is no
failure evidence requiring an increase. All 25 targeted tests passed,
checkstyle reported zero violations, and SpotBugs 4.9.7.0 completed with no
findings on the modified methods.
##########
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:
Renamed to testCompactionNotTriggered in dc82d36d541, covering both the
disabled collector and an SST deletion ratio below the configured threshold.
The ratio=1 case is intentional: the second SST also contains a put, so its
actual deletion ratio remains below 1.
##########
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:
Clarified the error message to say 'between 0 and 1 inclusive' in
dc82d36d541.
--
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]
