[GitHub] [gobblin] autumnust commented on a change in pull request #3400: [GOBBLIN-1550] add dataset.root in a couple of type of datasets

[GitBox <gi...@apache.org>]

autumnust commented on a change in pull request #3400:
URL: https://github.com/apache/gobblin/pull/3400#discussion_r714130245



##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/hive/HiveDataset.java
##########
@@ -26,11 +25,6 @@
 import java.util.Map;
 import java.util.Properties;
 
-import lombok.Getter;

Review comment:
       Was this change relevant ?

##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/hive/HiveDataset.java
##########
@@ -130,6 +129,10 @@ public HiveDataset(FileSystem fs, HiveMetastoreClientPool clientPool, Table tabl
 
     this.tableRootPath = PathUtils.isGlob(this.table.getDataLocation()) ? Optional.<Path> absent() :
         Optional.fromNullable(this.table.getDataLocation());
+    if (this.tableRootPath.isPresent()) {
+      this.properties.setProperty(FsCleanableHelper.RETENTION_DATASET_ROOT,

Review comment:
       this is leaking abstraction as `HiveDataset` is not only serving for retention job.




-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org