phet commented on code in PR #3687:
URL: https://github.com/apache/gobblin/pull/3687#discussion_r1178754124
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/version/HiveDatasetVersionCleaner.java:
##########
@@ -88,20 +88,24 @@ public void clean() throws IOException {
Partition partition = hiveDatasetVersion.getPartition();
try {
if (!cleanableHiveDataset.isSimulate()) {
+ if (cleanableHiveDataset.isShouldDeleteData()) {
+
cleanableHiveDataset.getFsCleanableHelper().clean(hiveDatasetVersion,
possiblyEmptyDirectories);
+ }
client.get().dropPartition(partition.getTable().getDbName(),
partition.getTable().getTableName(), partition.getValues(), false);
log.info("Successfully dropped partition " +
partition.getCompleteName());
} else {
log.info("Simulating drop partition " + partition.getCompleteName());
}
- if (cleanableHiveDataset.isShouldDeleteData()) {
-
cleanableHiveDataset.getFsCleanableHelper().clean(hiveDatasetVersion,
possiblyEmptyDirectories);
- }
} catch (TException | IOException e) {
log.warn(String.format("Failed to completely delete partition %s.",
partition.getCompleteName()), e);
throw new IOException(e);
}
+ } try {
Review Comment:
a `try` should begin on its own line
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/version/HiveDatasetVersionCleaner.java:
##########
@@ -88,20 +88,24 @@ public void clean() throws IOException {
Partition partition = hiveDatasetVersion.getPartition();
try {
if (!cleanableHiveDataset.isSimulate()) {
+ if (cleanableHiveDataset.isShouldDeleteData()) {
+
cleanableHiveDataset.getFsCleanableHelper().clean(hiveDatasetVersion,
possiblyEmptyDirectories);
+ }
client.get().dropPartition(partition.getTable().getDbName(),
partition.getTable().getTableName(), partition.getValues(), false);
Review Comment:
since this is tricky enough - to the extent that we're here fixing a logical
bug - please document w/ a comment that order really is critical (and why
that's so)
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/version/HiveDatasetVersionCleaner.java:
##########
@@ -88,20 +88,24 @@ public void clean() throws IOException {
Partition partition = hiveDatasetVersion.getPartition();
try {
if (!cleanableHiveDataset.isSimulate()) {
+ if (cleanableHiveDataset.isShouldDeleteData()) {
+
cleanableHiveDataset.getFsCleanableHelper().clean(hiveDatasetVersion,
possiblyEmptyDirectories);
Review Comment:
are we certain this is an idempotent operation that won't throw an exception
the second and more time?
scenario:
1. the first time through it was deleted before dropping of the partition
failed
2. upon retry, before again trying to drop the partition, we invoke this
once more to delete a FS location that was already removed
--
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]
