arjun4084346 commented on code in PR #4036:
URL: https://github.com/apache/gobblin/pull/4036#discussion_r1729309787


##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/trash/Trash.java:
##########
@@ -189,11 +197,18 @@ public boolean moveToTrash(Path path) throws IOException {
     Path targetPathInTrash = PathUtils.mergePaths(this.trashLocation, 
fullyResolvedPath);
 
     if (!this.fs.exists(targetPathInTrash.getParent())) {
-      this.fs.mkdirs(targetPathInTrash.getParent());
+      if (this.simulate) {
+        LOG.info("Making a parent directory at " + 
targetPathInTrash.getParent() + " in trash.");
+      } else {
+        this.fs.mkdirs(targetPathInTrash.getParent());
+      }
     } else if (this.fs.exists(targetPathInTrash)) {
       targetPathInTrash = targetPathInTrash.suffix("_" + 
System.currentTimeMillis());
     }
-
+    if (this.simulate) {
+      LOG.info("Simulating moving " + fullyResolvedPath + " to " + 
targetPathInTrash + " in trash.");
+      return true;
+    }

Review Comment:
   * nit , i prefer doing it like `else { return ...}` to emphasize on the need 
that only one blocks should run. in case we needed to add a line before L212, 
it may not by mistake be added to only one branch



-- 
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]

Reply via email to