nsivabalan commented on a change in pull request #4212:
URL: https://github.com/apache/hudi/pull/4212#discussion_r803776863
##########
File path:
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java
##########
@@ -1737,6 +1739,75 @@ public void testErrorCases() throws Exception {
}
}
+ /**
+ * Tests no more than 1 clean is scheduled/executed if
HoodieCompactionConfig.allowMultipleCleanSchedule config is disabled.
+ */
+ @Test
+ public void testMultiClean() throws Exception {
Review comment:
yeah. I just followed up from where you left :) will try to move it to
TestCleaner.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -721,21 +721,29 @@ public HoodieCleanMetadata clean(String cleanInstantTime,
boolean skipLocking) t
* @param skipLocking if this is triggered by another parent transaction,
locking can be skipped.
*/
public HoodieCleanMetadata clean(String cleanInstantTime, boolean
scheduleInline, boolean skipLocking) throws HoodieIOException {
- if (scheduleInline) {
- scheduleTableServiceInternal(cleanInstantTime, Option.empty(),
TableServiceType.CLEAN);
- }
LOG.info("Cleaner started");
final Timer.Context timerContext = metrics.getCleanCtx();
LOG.info("Cleaned failed attempts if any");
Review comment:
I have moved it to CleanerUtils.rollbackFailedWrites().
rollbackFailedwrites() is called in regular rollbacks as well (single
writer) and not in the context of cleaner
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java
##########
@@ -254,6 +254,12 @@
.withDocumentation("The average record size. If not explicitly
specified, hudi will compute the "
+ "record size estimate compute dynamically based on commit
metadata. "
+ " This is critical in computing the insert parallelism and
bin-packing inserts into small files.");
+
+ public static final ConfigProperty<Boolean> ALLOW_MULTIPLE_CLEANS =
ConfigProperty
+ .key("hoodie.allow.multiple.cleans")
Review comment:
In general, its a good practice to name the variable as you read it. for
eg, the getter method of this variable reads as allowMultipleCleans() which
goes in line w/ the config key.
If you check few other existing configs, it does align well. but I want to
fix that going forward. for eg:hoodie.clean.automatic , variable is called
AUTO_CLEAN and method is called isAutoClean. I would have preferred the config
key to be "hoodie.auto.clean".
--
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]
