xushiyan commented on code in PR #7196:
URL: https://github.com/apache/hudi/pull/7196#discussion_r1032384887
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java:
##########
@@ -267,21 +267,27 @@ public void deleteCompactionRequested(HoodieInstant
instant) {
}
public void deleteInstantFileIfExists(HoodieInstant instant) {
+ deleteInstantFileIfExists(instant, true);
+ }
+
+ public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean
exceptionIfFailToDelete) {
Review Comment:
```suggestion
public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean
throwIfFailed) {
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java:
##########
@@ -570,12 +570,24 @@ private boolean
deleteArchivedInstants(List<HoodieInstant> archivedInstants, Hoo
return success;
}
- private boolean deleteArchivedInstantFiles(HoodieEngineContext context,
boolean success, List<String> files) {
- Map<String, Boolean> resultDeleteInstantFiles =
deleteFilesParallelize(metaClient, files, context, false);
+ private boolean deleteArchivedInstants(HoodieEngineContext context,
+ HoodieTableMetaClient metaClient,
+ List<HoodieInstant> instants) {
+ if (instants.isEmpty()) {
+ return true;
+ }
- for (Map.Entry<String, Boolean> result :
resultDeleteInstantFiles.entrySet()) {
- LOG.info("Archived and deleted instant file " + result.getKey() + " : "
+ result.getValue());
- success &= result.getValue();
+ Map<HoodieInstant, Boolean> result = context.mapToPair(
+ instants,
+ instant -> ImmutablePair.of(
Review Comment:
hide implementation: use `Pair.of()`
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java:
##########
@@ -267,21 +267,27 @@ public void deleteCompactionRequested(HoodieInstant
instant) {
}
public void deleteInstantFileIfExists(HoodieInstant instant) {
+ deleteInstantFileIfExists(instant, true);
+ }
+
+ public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean
exceptionIfFailToDelete) {
Review Comment:
in what case do we want to silence the failure? this relates to timeline
integrity so we should prefer to fail out loud
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java:
##########
@@ -267,21 +267,27 @@ public void deleteCompactionRequested(HoodieInstant
instant) {
}
public void deleteInstantFileIfExists(HoodieInstant instant) {
+ deleteInstantFileIfExists(instant, true);
+ }
+
+ public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean
exceptionIfFailToDelete) {
LOG.info("Deleting instant " + instant);
- Path inFlightCommitFilePath =
getInstantFileNamePath(instant.getFileName());
+ Path commitFilePath = getInstantFileNamePath(instant.getFileName());
try {
- if (metaClient.getFs().exists(inFlightCommitFilePath)) {
- boolean result = metaClient.getFs().delete(inFlightCommitFilePath,
false);
+ if (metaClient.getFs().exists(commitFilePath)) {
+ boolean result = metaClient.getFs().delete(commitFilePath, false);
if (result) {
LOG.info("Removed instant " + instant);
- } else {
+ } else if (exceptionIfFailToDelete) {
Review Comment:
again, i don't think we should allow silencing exception at this level
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java:
##########
@@ -570,12 +570,24 @@ private boolean
deleteArchivedInstants(List<HoodieInstant> archivedInstants, Hoo
return success;
}
- private boolean deleteArchivedInstantFiles(HoodieEngineContext context,
boolean success, List<String> files) {
- Map<String, Boolean> resultDeleteInstantFiles =
deleteFilesParallelize(metaClient, files, context, false);
+ private boolean deleteArchivedInstants(HoodieEngineContext context,
+ HoodieTableMetaClient metaClient,
+ List<HoodieInstant> instants) {
+ if (instants.isEmpty()) {
+ return true;
+ }
- for (Map.Entry<String, Boolean> result :
resultDeleteInstantFiles.entrySet()) {
- LOG.info("Archived and deleted instant file " + result.getKey() + " : "
+ result.getValue());
- success &= result.getValue();
+ Map<HoodieInstant, Boolean> result = context.mapToPair(
+ instants,
+ instant -> ImmutablePair.of(
+ instant,
+ metaClient.getActiveTimeline().deleteInstantFileIfExists(instant,
false)),
+ Math.min(instants.size(), config.getArchiveDeleteParallelism()));
+
+ boolean success = true;
+ for (Map.Entry<HoodieInstant, Boolean> entry : result.entrySet()) {
+ LOG.info("Archived and deleted instant " + entry.getKey().toString() + "
: " + entry.getValue());
+ success &= entry.getValue();
Review Comment:
you can chain this with `result` using stream API `allMatch()`
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java:
##########
@@ -267,21 +267,27 @@ public void deleteCompactionRequested(HoodieInstant
instant) {
}
public void deleteInstantFileIfExists(HoodieInstant instant) {
+ deleteInstantFileIfExists(instant, true);
+ }
+
+ public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean
exceptionIfFailToDelete) {
LOG.info("Deleting instant " + instant);
- Path inFlightCommitFilePath =
getInstantFileNamePath(instant.getFileName());
+ Path commitFilePath = getInstantFileNamePath(instant.getFileName());
Review Comment:
we need to be careful about what commit can be deleted. this API is designed
to only delete requested/inflight or empty clean commit, based on the usage. (1
exception is in
`org.apache.hudi.cli.commands.TestRepairsCommand#testShowFailedCommits` where
this api is misused for deleting completed commits in tests; we should make
separate test helper for that)
We can't delete completed commit instants, which breaks the timeline's
integrity. So we should name it properly at the variable as well as the API
level.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java:
##########
@@ -570,12 +570,24 @@ private boolean
deleteArchivedInstants(List<HoodieInstant> archivedInstants, Hoo
return success;
}
- private boolean deleteArchivedInstantFiles(HoodieEngineContext context,
boolean success, List<String> files) {
- Map<String, Boolean> resultDeleteInstantFiles =
deleteFilesParallelize(metaClient, files, context, false);
+ private boolean deleteArchivedInstants(HoodieEngineContext context,
+ HoodieTableMetaClient metaClient,
+ List<HoodieInstant> instants) {
+ if (instants.isEmpty()) {
+ return true;
+ }
- for (Map.Entry<String, Boolean> result :
resultDeleteInstantFiles.entrySet()) {
- LOG.info("Archived and deleted instant file " + result.getKey() + " : "
+ result.getValue());
- success &= result.getValue();
+ Map<HoodieInstant, Boolean> result = context.mapToPair(
+ instants,
+ instant -> ImmutablePair.of(
+ instant,
+ metaClient.getActiveTimeline().deleteInstantFileIfExists(instant,
false)),
Review Comment:
the original invocation is `ignoreFailure=false`; you inverted the flag here
by saying `exceptionIfFailToDelete=false`, which silence the error
--
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]
