You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/10/24 21:09:38 UTC

[GitHub] [incubator-gobblin] arjun4084346 opened a new pull request #3140: add COMBINE_RETENTION_POLICIES config

arjun4084346 opened a new pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140


   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   
   ### JIRA
   - [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-XXX
   
   
   ### Description
   - [ ] Here are some details about my PR, including screenshots (if applicable):
   
   
   ### Tests
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   
   
   ### Commits
   - [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   


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

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

[GitHub] [incubator-gobblin] codecov-io edited a comment on pull request #3140: [GOBBLIN-1302] add COMBINE_RETENTION_POLICIES config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140#issuecomment-716059650


   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=h1) Report
   > Merging [#3140](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/6e9bb2af7e8ee8c482bf94f3c323c5bd76c748ff?el=desc) will **increase** coverage by `0.01%`.
   > The diff coverage is `84.21%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3140      +/-   ##
   ============================================
   + Coverage     46.02%   46.03%   +0.01%     
   - Complexity     9580     9591      +11     
   ============================================
     Files          1986     1986              
     Lines         75795    75819      +24     
     Branches       8445     8446       +1     
   ============================================
   + Hits          34884    34903      +19     
   - Misses        37634    37639       +5     
     Partials       3277     3277              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...ent/retention/policy/DeleteAllRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvRGVsZXRlQWxsUmV0ZW50aW9uUG9saWN5LmphdmE=) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...retention/policy/DeleteNothingRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvRGVsZXRlTm90aGluZ1JldGVudGlvblBvbGljeS5qYXZh) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...ement/retention/policy/NewestKRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvTmV3ZXN0S1JldGVudGlvblBvbGljeS5qYXZh) | `92.00% <ø> (ø)` | `9.00 <0.00> (ø)` | |
   | [...ent/retention/policy/PredicateRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvUHJlZGljYXRlUmV0ZW50aW9uUG9saWN5LmphdmE=) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...ent/retention/policy/TimeBasedRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvVGltZUJhc2VkUmV0ZW50aW9uUG9saWN5LmphdmE=) | `96.00% <ø> (ø)` | `10.00 <0.00> (ø)` | |
   | [.../java/org/apache/gobblin/util/PropertiesUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvUHJvcGVydGllc1V0aWxzLmphdmE=) | `34.14% <75.00%> (+4.41%)` | `8.00 <2.00> (+2.00)` | |
   | [...ement/retention/policy/CombineRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvQ29tYmluZVJldGVudGlvblBvbGljeS5qYXZh) | `75.38% <86.66%> (+2.05%)` | `17.00 <4.00> (+2.00)` | |
   | [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `92.85% <0.00%> (-7.15%)` | `4.00% <0.00%> (ø%)` | |
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `32.23% <0.00%> (-5.79%)` | `12.00% <0.00%> (-2.00%)` | |
   | [...a/org/apache/gobblin/service/FlowConfigClient.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnQ2xpZW50LmphdmE=) | `71.66% <0.00%> (-2.48%)` | `8.00% <0.00%> (ø%)` | |
   | ... and [17 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=footer). Last update [6e9bb2a...ca82e80](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

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

[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3140: [GOBBLIN-1302] add COMBINE_RETENTION_POLICIES config

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140#discussion_r512209220



##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java
##########
@@ -204,7 +204,7 @@ static void waitJobCompletion(HelixManager helixManager, String workFlowName, St
       } else {
         // We have waited for WorkflowContext to get initialized,
         // so it is found null here, it must have been deleted in job cancellation process.
-        log.info("WorkflowContext not found. Job is probably cancelled.");
+        log.info("WorkflowContext not found. Job {} is probably cancelled.", jobName);

Review comment:
       I added this in the description. Do you want me to create a separate PR for this?




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

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

[GitHub] [incubator-gobblin] codecov-io edited a comment on pull request #3140: [GOBBLIN-1302] add COMBINE_RETENTION_POLICIES config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140#issuecomment-716059650


   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=h1) Report
   > Merging [#3140](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/6e9bb2af7e8ee8c482bf94f3c323c5bd76c748ff?el=desc) will **increase** coverage by `0.01%`.
   > The diff coverage is `80.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3140      +/-   ##
   ============================================
   + Coverage     46.02%   46.03%   +0.01%     
   - Complexity     9580     9589       +9     
   ============================================
     Files          1986     1986              
     Lines         75795    75819      +24     
     Branches       8445     8446       +1     
   ============================================
   + Hits          34884    34906      +22     
   + Misses        37634    37633       -1     
   - Partials       3277     3280       +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `34.71% <0.00%> (-3.31%)` | `13.00 <0.00> (-1.00)` | |
   | [...ent/retention/policy/DeleteAllRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvRGVsZXRlQWxsUmV0ZW50aW9uUG9saWN5LmphdmE=) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...retention/policy/DeleteNothingRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvRGVsZXRlTm90aGluZ1JldGVudGlvblBvbGljeS5qYXZh) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...ement/retention/policy/NewestKRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvTmV3ZXN0S1JldGVudGlvblBvbGljeS5qYXZh) | `92.00% <ø> (ø)` | `9.00 <0.00> (ø)` | |
   | [...ent/retention/policy/PredicateRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvUHJlZGljYXRlUmV0ZW50aW9uUG9saWN5LmphdmE=) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...ent/retention/policy/TimeBasedRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvVGltZUJhc2VkUmV0ZW50aW9uUG9saWN5LmphdmE=) | `96.00% <ø> (ø)` | `10.00 <0.00> (ø)` | |
   | [.../java/org/apache/gobblin/util/PropertiesUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvUHJvcGVydGllc1V0aWxzLmphdmE=) | `34.14% <75.00%> (+4.41%)` | `8.00 <2.00> (+2.00)` | |
   | [...ement/retention/policy/CombineRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvQ29tYmluZVJldGVudGlvblBvbGljeS5qYXZh) | `75.38% <86.66%> (+2.05%)` | `17.00 <4.00> (+2.00)` | |
   | [...a/org/apache/gobblin/service/FlowConfigClient.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnQ2xpZW50LmphdmE=) | `71.66% <0.00%> (-2.48%)` | `8.00% <0.00%> (ø%)` | |
   | ... and [17 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=footer). Last update [6e9bb2a...5127729](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

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

[GitHub] [incubator-gobblin] asfgit closed pull request #3140: [GOBBLIN-1302] add COMBINE_RETENTION_POLICIES config

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140


   


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

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

[GitHub] [incubator-gobblin] codecov-io commented on pull request #3140: add COMBINE_RETENTION_POLICIES config

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140#issuecomment-716059650


   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=h1) Report
   > Merging [#3140](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/6e9bb2af7e8ee8c482bf94f3c323c5bd76c748ff?el=desc) will **increase** coverage by `0.01%`.
   > The diff coverage is `61.11%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3140      +/-   ##
   ============================================
   + Coverage     46.02%   46.03%   +0.01%     
   - Complexity     9580     9589       +9     
   ============================================
     Files          1986     1986              
     Lines         75795    75817      +22     
     Branches       8445     8446       +1     
   ============================================
   + Hits          34884    34902      +18     
   - Misses        37634    37637       +3     
   - Partials       3277     3278       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `38.01% <0.00%> (ø)` | `14.00 <0.00> (ø)` | |
   | [.../java/org/apache/gobblin/util/PropertiesUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvUHJvcGVydGllc1V0aWxzLmphdmE=) | `26.82% <0.00%> (-2.91%)` | `6.00 <0.00> (ø)` | |
   | [...ement/retention/policy/CombineRetentionPolicy.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3JldGVudGlvbi9wb2xpY3kvQ29tYmluZVJldGVudGlvblBvbGljeS5qYXZh) | `74.60% <84.61%> (+1.26%)` | `17.00 <4.00> (+2.00)` | |
   | [...a/org/apache/gobblin/service/FlowConfigClient.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnQ2xpZW50LmphdmE=) | `71.66% <0.00%> (-2.48%)` | `8.00% <0.00%> (ø%)` | |
   | [...apache/gobblin/kafka/writer/Kafka09DataWriter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMDkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4va2Fma2Evd3JpdGVyL0thZmthMDlEYXRhV3JpdGVyLmphdmE=) | `72.30% <0.00%> (-2.27%)` | `10.00% <0.00%> (ø%)` | |
   | [...org/apache/gobblin/service/FlowConfigV2Client.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnVjJDbGllbnQuamF2YQ==) | `58.88% <0.00%> (-1.34%)` | `10.00% <0.00%> (ø%)` | |
   | [...c/main/java/org/apache/gobblin/util/PortUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvUG9ydFV0aWxzLmphdmE=) | `74.19% <0.00%> (-1.22%)` | `14.00% <0.00%> (ø%)` | |
   | [...a/org/apache/gobblin/cluster/GobblinHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4VGFzay5qYXZh) | `60.21% <0.00%> (-0.66%)` | `6.00% <0.00%> (ø%)` | |
   | [...n/java/org/apache/gobblin/util/PullFileLoader.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvUHVsbEZpbGVMb2FkZXIuamF2YQ==) | `73.88% <0.00%> (-0.34%)` | `26.00% <0.00%> (+1.00%)` | :arrow_down: |
   | [.../org/apache/gobblin/cluster/GobblinTaskRunner.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpblRhc2tSdW5uZXIuamF2YQ==) | `63.46% <0.00%> (-0.33%)` | `35.00% <0.00%> (+1.00%)` | :arrow_down: |
   | ... and [9 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3140/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=footer). Last update [6e9bb2a...705c0d6](https://codecov.io/gh/apache/incubator-gobblin/pull/3140?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

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

[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3140: [GOBBLIN-1302] add COMBINE_RETENTION_POLICIES config

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140#discussion_r512210806



##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/policy/CombineRetentionPolicy.java
##########
@@ -110,6 +105,30 @@ public CombineRetentionPolicy(Properties props) throws IOException {
 
   }
 
+  private List<RetentionPolicy<T>> findRetentionPolicies(Properties props) {
+    List<String> retentionPolicyClasses;
+    ImmutableList.Builder<RetentionPolicy<T>> builder = ImmutableList.builder();
+    ClassAliasResolver<?> aliasResolver = new ClassAliasResolver<>(RetentionPolicy.class);
+
+    if (props.containsKey(COMBINE_RETENTION_POLICIES)) {
+      retentionPolicyClasses = COMMA_BASED_SPLITTER.splitToList(props.getProperty(COMBINE_RETENTION_POLICIES));

Review comment:
       It is there at Line 78




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

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

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3140: [GOBBLIN-1302] add COMBINE_RETENTION_POLICIES config

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140#discussion_r512267477



##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/policy/CombineRetentionPolicy.java
##########
@@ -110,6 +105,30 @@ public CombineRetentionPolicy(Properties props) throws IOException {
 
   }
 
+  private List<RetentionPolicy<T>> findRetentionPolicies(Properties props) {
+    List<String> retentionPolicyClasses;
+    ImmutableList.Builder<RetentionPolicy<T>> builder = ImmutableList.builder();
+    ClassAliasResolver<?> aliasResolver = new ClassAliasResolver<>(RetentionPolicy.class);
+
+    if (props.containsKey(COMBINE_RETENTION_POLICIES)) {
+      retentionPolicyClasses = COMMA_BASED_SPLITTER.splitToList(props.getProperty(COMBINE_RETENTION_POLICIES));

Review comment:
       Ack. Thanks!

##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java
##########
@@ -204,7 +204,7 @@ static void waitJobCompletion(HelixManager helixManager, String workFlowName, St
       } else {
         // We have waited for WorkflowContext to get initialized,
         // so it is found null here, it must have been deleted in job cancellation process.
-        log.info("WorkflowContext not found. Job is probably cancelled.");
+        log.info("WorkflowContext not found. Job {} is probably cancelled.", jobName);

Review comment:
       Even though it is a trivial one-line change, I prefer to keep unrelated changes in separate PRs. The context of such unrelated changes is lost after some time and can be confusing to a new developer. My suggestion is to pull this change out of this PR. 




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

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

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3140: [GOBBLIN-1302] add COMBINE_RETENTION_POLICIES config

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3140:
URL: https://github.com/apache/incubator-gobblin/pull/3140#discussion_r512081332



##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/policy/CombineRetentionPolicy.java
##########
@@ -62,10 +66,16 @@
  */
 public class CombineRetentionPolicy<T extends DatasetVersion> implements RetentionPolicy<T> {
 
+  public static final String COMBINE_RETENTION_POLICIES =
+      DatasetCleaner.CONFIGURATION_KEY_PREFIX + "combine.retention.policy.classes";
+  /**
+   * @Deprecated , use COMBINE_RETENTION_POLICIES instead.

Review comment:
       +1. 

##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java
##########
@@ -204,7 +204,7 @@ static void waitJobCompletion(HelixManager helixManager, String workFlowName, St
       } else {
         // We have waited for WorkflowContext to get initialized,
         // so it is found null here, it must have been deleted in job cancellation process.
-        log.info("WorkflowContext not found. Job is probably cancelled.");
+        log.info("WorkflowContext not found. Job {} is probably cancelled.", jobName);

Review comment:
       Seems unrelated to the change proposed in this PR. Can we remove this?

##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/policy/CombineRetentionPolicy.java
##########
@@ -110,6 +105,30 @@ public CombineRetentionPolicy(Properties props) throws IOException {
 
   }
 
+  private List<RetentionPolicy<T>> findRetentionPolicies(Properties props) {
+    List<String> retentionPolicyClasses;
+    ImmutableList.Builder<RetentionPolicy<T>> builder = ImmutableList.builder();
+    ClassAliasResolver<?> aliasResolver = new ClassAliasResolver<>(RetentionPolicy.class);
+
+    if (props.containsKey(COMBINE_RETENTION_POLICIES)) {
+      retentionPolicyClasses = COMMA_BASED_SPLITTER.splitToList(props.getProperty(COMBINE_RETENTION_POLICIES));
+    } else {
+      retentionPolicyClasses = PropertiesUtils.getValuesAsList(props, Optional.of(RETENTION_POLICIES_PREFIX));
+    }
+
+    for (String retentionPolicyClass : retentionPolicyClasses) {
+      try {
+        builder.add((RetentionPolicy<T>) ConstructorUtils.invokeConstructor(
+            Class.forName(aliasResolver.resolve(retentionPolicyClass)), props));
+      } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException | InstantiationException

Review comment:
       catch can be simplified to catch(ReflectiveOperationException).

##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/policy/CombineRetentionPolicy.java
##########
@@ -110,6 +105,30 @@ public CombineRetentionPolicy(Properties props) throws IOException {
 
   }
 
+  private List<RetentionPolicy<T>> findRetentionPolicies(Properties props) {
+    List<String> retentionPolicyClasses;
+    ImmutableList.Builder<RetentionPolicy<T>> builder = ImmutableList.builder();
+    ClassAliasResolver<?> aliasResolver = new ClassAliasResolver<>(RetentionPolicy.class);
+
+    if (props.containsKey(COMBINE_RETENTION_POLICIES)) {
+      retentionPolicyClasses = COMMA_BASED_SPLITTER.splitToList(props.getProperty(COMBINE_RETENTION_POLICIES));

Review comment:
       We should strip leading and trailing whitespaces using Splitter.on(',').trimResults().splitToList(...).

##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/policy/NewestKRetentionPolicy.java
##########
@@ -27,13 +27,15 @@
 import com.google.common.collect.Lists;
 import com.typesafe.config.Config;
 
+import org.apache.gobblin.annotation.Alias;
 import org.apache.gobblin.data.management.retention.DatasetCleaner;
 import org.apache.gobblin.data.management.version.DatasetVersion;
 
 
 /**
  * Retains the newest k versions of the dataset.
  */
+@Alias("NewestK")

Review comment:
       +1. Can we go all the way and add aliases to other retention policies too? There must be 3-4 more of them. 

##########
File path: gobblin-data-management/src/main/java/org/apache/gobblin/data/management/retention/policy/CombineRetentionPolicy.java
##########
@@ -110,6 +105,30 @@ public CombineRetentionPolicy(Properties props) throws IOException {
 
   }
 
+  private List<RetentionPolicy<T>> findRetentionPolicies(Properties props) {
+    List<String> retentionPolicyClasses;
+    ImmutableList.Builder<RetentionPolicy<T>> builder = ImmutableList.builder();
+    ClassAliasResolver<?> aliasResolver = new ClassAliasResolver<>(RetentionPolicy.class);
+
+    if (props.containsKey(COMBINE_RETENTION_POLICIES)) {
+      retentionPolicyClasses = COMMA_BASED_SPLITTER.splitToList(props.getProperty(COMBINE_RETENTION_POLICIES));

Review comment:
       Perhaps a good idea to do omitEmptyStrings() too. 

##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/PropertiesUtils.java
##########
@@ -84,6 +84,20 @@ public static long getPropAsLong(Properties properties, String key, long default
     return LIST_SPLITTER.splitToList(properties.getProperty(key));
   }
 
+  /**
+   * Extract all the values whose keys start with a <code>prefix</code>
+   * @param properties the given {@link Properties} instance
+   * @param prefix of keys to be extracted
+   * @return a list of values in the properties
+   */
+  public static List<String> getValuesAsList(Properties properties, Optional<String> prefix) {

Review comment:
       Can we add a unit test for this method? 




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

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