sarvekshayr commented on code in PR #10419:
URL: https://github.com/apache/ozone/pull/10419#discussion_r3371012063
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactDBUtil.java:
##########
@@ -38,13 +38,13 @@ public final class CompactDBUtil {
private CompactDBUtil() {
}
- public static void compactTable(OMMetadataManager omMetadataManager,
- String tableName) throws IOException {
+ public static void compactTable(OMMetadataManager omMetadataManager, String
tableName,
+ ManagedCompactRangeOptions.BottommostLevelCompaction compactionType)
throws IOException {
long startTime = Time.monotonicNow();
LOG.info("Compacting column family: {}", tableName);
Review Comment:
It would be better to include the compaction level in this INFO log.
```suggestion
LOG.info("Compacting column family: {} with bottommost level compaction:
{}", tableName, compactionType);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactDBUtil.java:
##########
@@ -38,13 +38,13 @@ public final class CompactDBUtil {
private CompactDBUtil() {
}
- public static void compactTable(OMMetadataManager omMetadataManager,
- String tableName) throws IOException {
+ public static void compactTable(OMMetadataManager omMetadataManager, String
tableName,
+ ManagedCompactRangeOptions.BottommostLevelCompaction compactionType)
throws IOException {
long startTime = Time.monotonicNow();
LOG.info("Compacting column family: {}", tableName);
try (ManagedCompactRangeOptions options = new
ManagedCompactRangeOptions()) {
- options.setBottommostLevelCompaction(
- ManagedCompactRangeOptions.BottommostLevelCompaction.kForce);
+ options.setBottommostLevelCompaction(compactionType);
+ LOG.debug("Setting bottommost level compaction type to {}",
options.bottommostLevelCompaction());
Review Comment:
This DEBUG log can be removed then.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactionService.java:
##########
@@ -65,6 +68,16 @@ public CompactionService(OzoneManager ozoneManager, TimeUnit
unit, long interval
this.numCompactions = new AtomicLong(0);
this.suspended = new AtomicBoolean(false);
this.compactableTables = validateTables(tables);
+ int compactionType = Integer.parseInt(ozoneManager.getConfiguration().get(
+ OMConfigKeys.OZONE_OM_COMPACTION_SERVICE_BOTTOMMOSTLEVELCOMPACTION,
+
OMConfigKeys.OZONE_OM_COMPACTION_SERVICE_BOTTOMMOSTLEVELCOMPACTION_DEFAULT));
+ ManagedCompactRangeOptions.BottommostLevelCompaction level =
+
ManagedCompactRangeOptions.BottommostLevelCompaction.fromRocksId(compactionType);
+ if (level == null) {
+ compactionType =
Integer.parseInt(OMConfigKeys.OZONE_OM_COMPACTION_SERVICE_BOTTOMMOSTLEVELCOMPACTION_DEFAULT);
+ level =
ManagedCompactRangeOptions.BottommostLevelCompaction.fromRocksId(compactionType);
+ }
+ this.bottommostLevelCompaction = level;
Review Comment:
Instead of silently falling back to default `kSkip` when an invalid value is
provided, we should use fail-fast approach. Throw an `IllegalArgumentException`
with a clear message listing valid values if `Integer.parseInt()` fails (e.g.,
on non-numeric strings) or if `fromRocksId()` returns null (after changing to
string parsing add the check in the corresponding methods).
Alternatively, we could LOG the invalid value and fall back to the default
explicitly, but throwing an exception brings immediate attention to invalid
configuration during background compaction.
--
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]
