{"action":"created","comment":{"url":"https://api.github.com/repos/linkedin/gobblin/pulls/comments/81220259","pull_request_review_id":2216765,"id":81220259,"diff_hunk":"@@ -91,23 +93,58 @@ private void executeOnVersion(DatasetVersion datasetVersion) throws IOException\n     // Perform action if it is a FileSystemDatasetVersion\n     if (datasetVersion instanceof FileSystemDatasetVersion) {\n       FileSystemDatasetVersion fsDatasetVersion = (FileSystemDatasetVersion) datasetVersion;\n-      for (Path path : fsDatasetVersion.getPaths()) {\n-\n-        if (this.fs.exists(path)) {\n \n-          // Update permissions if set in config\n-          if (this.permission.isPresent()) {\n-            FsPermission fsPermission = new FsPermission(this.permission.get());\n-            this.fs.setPermission(path, fsPermission);\n-            log.debug(\"Set permissions for {} to {}\", path.toString(), fsPermission);\n+      // If the version is filestatus aware, use the filestatus to ignore permissions update when the path already has\n+      // the desired permissions\n+      if (datasetVersion instanceof FileStatusAware) {\n+        for (FileStatus fileStatus : ((FileStatusAware)datasetVersion).getFileStatuses()) {\n+          if (needsPermissionsUpdate(fileStatus) || needsOwnerUpdate(fileStatus) || needsGroupUpdate(fileStatus)) {\n+            updatePermissionsAndOwner(fileStatus.getPath());\n           }\n+        }\n+      } else {\n+        for (Path path : fsDatasetVersion.getPaths()) {\n+          updatePermissionsAndOwner(path);\n+        }\n+      }\n+    }\n+  }\n \n-          // Update owner and group if set in config\n-          if (this.owner.isPresent() || this.group.isPresent()) {\n-            this.fs.setOwner(path, this.owner.orNull(), this.group.orNull());\n-            log.debug(\"Set owner and group for {} to {}:{}\", path.toString(), this.owner.orNull(),\n-                this.group.orNull());\n-          }\n+  private boolean needsPermissionsUpdate(FileStatus fileStatus) {\n+    return this.permission.isPresent() && !new FsPermission(this.permission.get()).equals(fileStatus.getPermission());","path":"gobblin-data-management/src/main/java/gobblin/data/management/retention/action/AccessControlAction.java","position":71,"original_position":71,"commit_id":"ee77ced1946ab361285a8c1ccd01ba9be5d17837","original_commit_id":"ee77ced1946ab361285a8c1ccd01ba9be5d17837","user":{"login":"ibuenros","id":1364112,"avatar_url":"https://avatars.githubusercontent.com/u/1364112?v=3","gravatar_id":"","url":"https://api.github.com/users/ibuenros","html_url":"https://github.com/ibuenros","followers_url":"https://api.github.com/users/ibuenros/followers","following_url":"https://api.github.com/users/ibuenros/following{/other_user}","gists_url":"https://api.github.com/users/ibuenros/gists{/gist_id}","starred_url":"https://api.github.com/users/ibuenros/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ibuenros/subscriptions","organizations_url":"https://api.github.com/users/ibuenros/orgs","repos_url":"https://api.github.com/users/ibuenros/repos","events_url":"https://api.github.com/users/ibuenros/events{/privacy}","received_events_url":"https://api.github.com/users/ibuenros/received_events","type":"User","site_admin":false},"body":"Can we store the actual `FsPermission` object instead of having to parse every time?","created_at":"2016-09-29T19:52:34Z","updated_at":"2016-09-29T20:00:07Z","html_url":"https://github.com/linkedin/gobblin/pull/1288#discussion_r81220259","pull_request_url":"https://api.github.com/repos/linkedin/gobblin/pulls/1288","_links":{"self":{"href":"https://api.github.com/repos/linkedin/gobblin/pulls/comments/81220259"},"html":{"href":"https://github.com/linkedin/gobblin/pull/1288#discussion_r81220259"},"pull_request":{"href":"https://api.github.com/repos/linkedin/gobblin/pulls/1288"}}},"pull_request":{"url":"https://api.github.com/repos/linkedin/gobblin/pulls/1288","id":87233295,"html_url":"https://github.com/linkedin/gobblin/pull/1288","diff_url":"https://github.com/linkedin/gobblin/pull/1288.diff","patch_url":"https://github.com/linkedin/gobblin/pull/1288.patch","issue_url":"https://api.github.com/repos/linkedin/gobblin/issues/1288","number":1288,"state":"open","locked":false,"title":"Make AccessControlAction to update permissions only when required","user":{"login":"pcadabam","id":1166372,"avatar_url":"https://avatars.githubusercontent.com/u/1166372?v=3","gravatar_id":"","url":"https://api.github.com/users/pcadabam","html_url":"https://github.com/pcadabam","followers_url":"https://api.github.com/users/pcadabam/followers","following_url":"https://api.github.com/users/pcadabam/following{/other_user}","gists_url":"https://api.github.com/users/pcadabam/gists{/gist_id}","starred_url":"https://api.github.com/users/pcadabam/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pcadabam/subscriptions","organizations_url":"https://api.github.com/users/pcadabam/orgs","repos_url":"https://api.github.com/users/pcadabam/repos","events_url":"https://api.github.com/users/pcadabam/events{/privacy}","received_events_url":"https://api.github.com/users/pcadabam/received_events","type":"User","site_admin":false},"body":"### Changes\r\nThis PR looks like a lot of changes but it is not. It has several unrelated changes in a single PR\r\n\r\n#### Major changes\r\n\r\n1. Major change is in `AccessControlAction` to not make unecessary `FileSystem` calls. With this change, `AccessControlAction` calls `fs.setPermission` and `fs.setOwner` if the permissions and owner of a path are different from what the user wants. To make this change I had to move around some code because the current `FileSystemDatasetVersion` does not know the `FileStatus` of a `DatasetVersion`. It only knows the `Path`.\r\n2. Currently `DatasetCleanerJob` only supports running retention on datasets that import a tag. The only way to run retention on multiple kinds of datasets  that import different tags is to have different jobs. I have added multi tag support in this pr. The same key `gobblin.retention.tag` is now a comma separated list of tags in the config store. (Internal ask from ops ETL-4855)\r\n\r\n\r\n#### Minor trivial changes\r\n\r\n1. Logging in retention was too verbose and useless. Moved some of them to debug. Added `@ToString` to all the version selection policies for more meaningful logs\r\n2. By default the hive based retention used to delete data as well. Looking at the use cases, it seems like the default should to not delete data. Changed it in `CleanableHiveDataset` and a few unit tests.\r\n3. Chagned the `DEFAULT_MAX_CONCURRENT_DATASETS_CLEANED` to 100 from 1000. Since we have more actions like `AccessControl`, I expect more namenode calls. There is no need to stress the namenode because retention is not a time critical job.\r\n4. Added simulate mode for `AccessControlAction`\r\n\r\n@ydai1124 and @ibuenros  can you review?","created_at":"2016-09-29T01:52:00Z","updated_at":"2016-09-29T20:00:07Z","closed_at":null,"merged_at":null,"merge_commit_sha":"de1304500019a89978247fdaa712e27880ca3373","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/linkedin/gobblin/pulls/1288/commits","review_comments_url":"https://api.github.com/repos/linkedin/gobblin/pulls/1288/comments","review_comment_url":"https://api.github.com/repos/linkedin/gobblin/pulls/comments{/number}","comments_url":"https://api.github.com/repos/linkedin/gobblin/issues/1288/comments","statuses_url":"https://api.github.com/repos/linkedin/gobblin/statuses/ee77ced1946ab361285a8c1ccd01ba9be5d17837","head":{"label":"pcadabam:retention","ref":"retention","sha":"ee77ced1946ab361285a8c1ccd01ba9be5d17837","user":{"login":"pcadabam","id":1166372,"avatar_url":"https://avatars.githubusercontent.com/u/1166372?v=3","gravatar_id":"","url":"https://api.github.com/users/pcadabam","html_url":"https://github.com/pcadabam","followers_url":"https://api.github.com/users/pcadabam/followers","following_url":"https://api.github.com/users/pcadabam/following{/other_user}","gists_url":"https://api.github.com/users/pcadabam/gists{/gist_id}","starred_url":"https://api.github.com/users/pcadabam/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pcadabam/subscriptions","organizations_url":"https://api.github.com/users/pcadabam/orgs","repos_url":"https://api.github.com/users/pcadabam/repos","events_url":"https://api.github.com/users/pcadabam/events{/privacy}","received_events_url":"https://api.github.com/users/pcadabam/received_events","type":"User","site_admin":false},"repo":{"id":36956709,"name":"gobblin","full_name":"pcadabam/gobblin","owner":{"login":"pcadabam","id":1166372,"avatar_url":"https://avatars.githubusercontent.com/u/1166372?v=3","gravatar_id":"","url":"https://api.github.com/users/pcadabam","html_url":"https://github.com/pcadabam","followers_url":"https://api.github.com/users/pcadabam/followers","following_url":"https://api.github.com/users/pcadabam/following{/other_user}","gists_url":"https://api.github.com/users/pcadabam/gists{/gist_id}","starred_url":"https://api.github.com/users/pcadabam/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pcadabam/subscriptions","organizations_url":"https://api.github.com/users/pcadabam/orgs","repos_url":"https://api.github.com/users/pcadabam/repos","events_url":"https://api.github.com/users/pcadabam/events{/privacy}","received_events_url":"https://api.github.com/users/pcadabam/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/pcadabam/gobblin","description":"","fork":true,"url":"https://api.github.com/repos/pcadabam/gobblin","forks_url":"https://api.github.com/repos/pcadabam/gobblin/forks","keys_url":"https://api.github.com/repos/pcadabam/gobblin/keys{/key_id}","collaborators_url":"https://api.github.com/repos/pcadabam/gobblin/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/pcadabam/gobblin/teams","hooks_url":"https://api.github.com/repos/pcadabam/gobblin/hooks","issue_events_url":"https://api.github.com/repos/pcadabam/gobblin/issues/events{/number}","events_url":"https://api.github.com/repos/pcadabam/gobblin/events","assignees_url":"https://api.github.com/repos/pcadabam/gobblin/assignees{/user}","branches_url":"https://api.github.com/repos/pcadabam/gobblin/branches{/branch}","tags_url":"https://api.github.com/repos/pcadabam/gobblin/tags","blobs_url":"https://api.github.com/repos/pcadabam/gobblin/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/pcadabam/gobblin/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/pcadabam/gobblin/git/refs{/sha}","trees_url":"https://api.github.com/repos/pcadabam/gobblin/git/trees{/sha}","statuses_url":"https://api.github.com/repos/pcadabam/gobblin/statuses/{sha}","languages_url":"https://api.github.com/repos/pcadabam/gobblin/languages","stargazers_url":"https://api.github.com/repos/pcadabam/gobblin/stargazers","contributors_url":"https://api.github.com/repos/pcadabam/gobblin/contributors","subscribers_url":"https://api.github.com/repos/pcadabam/gobblin/subscribers","subscription_url":"https://api.github.com/repos/pcadabam/gobblin/subscription","commits_url":"https://api.github.com/repos/pcadabam/gobblin/commits{/sha}","git_commits_url":"https://api.github.com/repos/pcadabam/gobblin/git/commits{/sha}","comments_url":"https://api.github.com/repos/pcadabam/gobblin/comments{/number}","issue_comment_url":"https://api.github.com/repos/pcadabam/gobblin/issues/comments{/number}","contents_url":"https://api.github.com/repos/pcadabam/gobblin/contents/{+path}","compare_url":"https://api.github.com/repos/pcadabam/gobblin/compare/{base}...{head}","merges_url":"https://api.github.com/repos/pcadabam/gobblin/merges","archive_url":"https://api.github.com/repos/pcadabam/gobblin/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/pcadabam/gobblin/downloads","issues_url":"https://api.github.com/repos/pcadabam/gobblin/issues{/number}","pulls_url":"https://api.github.com/repos/pcadabam/gobblin/pulls{/number}","milestones_url":"https://api.github.com/repos/pcadabam/gobblin/milestones{/number}","notifications_url":"https://api.github.com/repos/pcadabam/gobblin/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/pcadabam/gobblin/labels{/name}","releases_url":"https://api.github.com/repos/pcadabam/gobblin/releases{/id}","deployments_url":"https://api.github.com/repos/pcadabam/gobblin/deployments","created_at":"2015-06-05T21:39:40Z","updated_at":"2016-01-28T20:06:08Z","pushed_at":"2016-09-29T17:36:19Z","git_url":"git://github.com/pcadabam/gobblin.git","ssh_url":"git@github.com:pcadabam/gobblin.git","clone_url":"https://github.com/pcadabam/gobblin.git","svn_url":"https://github.com/pcadabam/gobblin","homepage":null,"size":90708,"stargazers_count":0,"watchers_count":0,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"linkedin:master","ref":"master","sha":"7120a3e58d45409866cde532284e586bb41bd105","user":{"login":"linkedin","id":357098,"avatar_url":"https://avatars.githubusercontent.com/u/357098?v=3","gravatar_id":"","url":"https://api.github.com/users/linkedin","html_url":"https://github.com/linkedin","followers_url":"https://api.github.com/users/linkedin/followers","following_url":"https://api.github.com/users/linkedin/following{/other_user}","gists_url":"https://api.github.com/users/linkedin/gists{/gist_id}","starred_url":"https://api.github.com/users/linkedin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/linkedin/subscriptions","organizations_url":"https://api.github.com/users/linkedin/orgs","repos_url":"https://api.github.com/users/linkedin/repos","events_url":"https://api.github.com/users/linkedin/events{/privacy}","received_events_url":"https://api.github.com/users/linkedin/received_events","type":"Organization","site_admin":false},"repo":{"id":27392543,"name":"gobblin","full_name":"linkedin/gobblin","owner":{"login":"linkedin","id":357098,"avatar_url":"https://avatars.githubusercontent.com/u/357098?v=3","gravatar_id":"","url":"https://api.github.com/users/linkedin","html_url":"https://github.com/linkedin","followers_url":"https://api.github.com/users/linkedin/followers","following_url":"https://api.github.com/users/linkedin/following{/other_user}","gists_url":"https://api.github.com/users/linkedin/gists{/gist_id}","starred_url":"https://api.github.com/users/linkedin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/linkedin/subscriptions","organizations_url":"https://api.github.com/users/linkedin/orgs","repos_url":"https://api.github.com/users/linkedin/repos","events_url":"https://api.github.com/users/linkedin/events{/privacy}","received_events_url":"https://api.github.com/users/linkedin/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/linkedin/gobblin","description":"Universal data ingestion framework for Hadoop.","fork":false,"url":"https://api.github.com/repos/linkedin/gobblin","forks_url":"https://api.github.com/repos/linkedin/gobblin/forks","keys_url":"https://api.github.com/repos/linkedin/gobblin/keys{/key_id}","collaborators_url":"https://api.github.com/repos/linkedin/gobblin/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/linkedin/gobblin/teams","hooks_url":"https://api.github.com/repos/linkedin/gobblin/hooks","issue_events_url":"https://api.github.com/repos/linkedin/gobblin/issues/events{/number}","events_url":"https://api.github.com/repos/linkedin/gobblin/events","assignees_url":"https://api.github.com/repos/linkedin/gobblin/assignees{/user}","branches_url":"https://api.github.com/repos/linkedin/gobblin/branches{/branch}","tags_url":"https://api.github.com/repos/linkedin/gobblin/tags","blobs_url":"https://api.github.com/repos/linkedin/gobblin/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/linkedin/gobblin/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/linkedin/gobblin/git/refs{/sha}","trees_url":"https://api.github.com/repos/linkedin/gobblin/git/trees{/sha}","statuses_url":"https://api.github.com/repos/linkedin/gobblin/statuses/{sha}","languages_url":"https://api.github.com/repos/linkedin/gobblin/languages","stargazers_url":"https://api.github.com/repos/linkedin/gobblin/stargazers","contributors_url":"https://api.github.com/repos/linkedin/gobblin/contributors","subscribers_url":"https://api.github.com/repos/linkedin/gobblin/subscribers","subscription_url":"https://api.github.com/repos/linkedin/gobblin/subscription","commits_url":"https://api.github.com/repos/linkedin/gobblin/commits{/sha}","git_commits_url":"https://api.github.com/repos/linkedin/gobblin/git/commits{/sha}","comments_url":"https://api.github.com/repos/linkedin/gobblin/comments{/number}","issue_comment_url":"https://api.github.com/repos/linkedin/gobblin/issues/comments{/number}","contents_url":"https://api.github.com/repos/linkedin/gobblin/contents/{+path}","compare_url":"https://api.github.com/repos/linkedin/gobblin/compare/{base}...{head}","merges_url":"https://api.github.com/repos/linkedin/gobblin/merges","archive_url":"https://api.github.com/repos/linkedin/gobblin/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/linkedin/gobblin/downloads","issues_url":"https://api.github.com/repos/linkedin/gobblin/issues{/number}","pulls_url":"https://api.github.com/repos/linkedin/gobblin/pulls{/number}","milestones_url":"https://api.github.com/repos/linkedin/gobblin/milestones{/number}","notifications_url":"https://api.github.com/repos/linkedin/gobblin/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/linkedin/gobblin/labels{/name}","releases_url":"https://api.github.com/repos/linkedin/gobblin/releases{/id}","deployments_url":"https://api.github.com/repos/linkedin/gobblin/deployments","created_at":"2014-12-01T18:10:50Z","updated_at":"2016-09-29T17:59:58Z","pushed_at":"2016-09-29T18:52:46Z","git_url":"git://github.com/linkedin/gobblin.git","ssh_url":"git@github.com:linkedin/gobblin.git","clone_url":"https://github.com/linkedin/gobblin.git","svn_url":"https://github.com/linkedin/gobblin","homepage":"https://github.com/linkedin/gobblin/wiki","size":181357,"stargazers_count":731,"watchers_count":731,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":287,"mirror_url":null,"open_issues_count":121,"forks":287,"open_issues":121,"watchers":731,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/linkedin/gobblin/pulls/1288"},"html":{"href":"https://github.com/linkedin/gobblin/pull/1288"},"issue":{"href":"https://api.github.com/repos/linkedin/gobblin/issues/1288"},"comments":{"href":"https://api.github.com/repos/linkedin/gobblin/issues/1288/comments"},"review_comments":{"href":"https://api.github.com/repos/linkedin/gobblin/pulls/1288/comments"},"review_comment":{"href":"https://api.github.com/repos/linkedin/gobblin/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/linkedin/gobblin/pulls/1288/commits"},"statuses":{"href":"https://api.github.com/repos/linkedin/gobblin/statuses/ee77ced1946ab361285a8c1ccd01ba9be5d17837"}}}}
{"actor":{"display_login":"ibuenros"}}