phet commented on code in PR #3845:
URL: https://github.com/apache/gobblin/pull/3845#discussion_r1424664958


##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/ManifestBasedDataset.java:
##########
@@ -176,10 +176,11 @@ public Iterator<FileSet<CopyEntity>> 
getFileSetIterator(FileSystem targetFs, Cop
 
   private static boolean shouldCopy(FileSystem targetFs, FileStatus 
fileInSource, FileStatus fileInTarget, OwnerAndPermission replicatedPermission)
       throws IOException {
-    if (fileInSource.isDirectory() || fileInSource.getModificationTime() == 
fileInTarget.getModificationTime()) {
-      // if source is dir or source and dst has same version, we compare the 
permission to determine whether it needs another sync
+    if (fileInSource.isDirectory() || fileInSource.getModificationTime() <= 
fileInTarget.getModificationTime()) {
+      // even if destination has a newer version than the source, we still 
copy the file if the owner or permission is different
       return !replicatedPermission.hasSameOwnerAndPermission(targetFs, 
fileInTarget);
     }
-    return fileInSource.getModificationTime() > 
fileInTarget.getModificationTime();
+    // Source is newer than the target, must copy
+    return true;

Review Comment:
   would this be equivalent:
   ```
   return fileInSrc.modT() > fileInTarg.modT() || 
!replicatedPermission.hasSameOwnerAndPerms(targFs, fileInTarget);
   ```
   ?



##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/ManifestBasedDataset.java:
##########
@@ -176,10 +176,11 @@ public Iterator<FileSet<CopyEntity>> 
getFileSetIterator(FileSystem targetFs, Cop
 
   private static boolean shouldCopy(FileSystem targetFs, FileStatus 
fileInSource, FileStatus fileInTarget, OwnerAndPermission replicatedPermission)
       throws IOException {
-    if (fileInSource.isDirectory() || fileInSource.getModificationTime() == 
fileInTarget.getModificationTime()) {
-      // if source is dir or source and dst has same version, we compare the 
permission to determine whether it needs another sync
+    if (fileInSource.isDirectory() || fileInSource.getModificationTime() <= 
fileInTarget.getModificationTime()) {
+      // even if destination has a newer version than the source, we still 
copy the file if the owner or permission is different
       return !replicatedPermission.hasSameOwnerAndPermission(targetFs, 
fileInTarget);
     }
-    return fileInSource.getModificationTime() > 
fileInTarget.getModificationTime();
+    // Source is newer than the target, must copy
+    return true;

Review Comment:
   would this be equivalent:
   ```
   return fileInSrc.modT() > fileInTarg.modT()
       || !replicatedPermission.hasSameOwnerAndPerms(targFs, fileInTarget);
   ```
   ?



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