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 2021/01/20 23:41:07 UTC
[GitHub] [incubator-gobblin] ZihanLi58 opened a new pull request #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
ZihanLi58 opened a new pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207
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-1386
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Unit test also run yarn application to make sure the function works as expected
### 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 commented on pull request #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#issuecomment-764446157
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=h1) Report
> Merging [#3207](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=desc) (9709cbd) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/2fa3553c75775d2d2a3a98f51c92b5c758c7abc9?el=desc) (2fa3553) will **decrease** coverage by `4.31%`.
> The diff coverage is `14.28%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3207 +/- ##
============================================
- Coverage 46.23% 41.92% -4.32%
+ Complexity 9788 8935 -853
============================================
Files 2019 2019
Lines 77439 77539 +100
Branches 8602 8609 +7
============================================
- Hits 35807 32506 -3301
- Misses 38300 41970 +3670
+ Partials 3332 3063 -269
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...che/gobblin/yarn/GobblinYarnConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `66.66% <ø> (ø)` | `1.00 <0.00> (ø)` | |
| [...main/java/org/apache/gobblin/yarn/YarnService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFyblNlcnZpY2UuamF2YQ==) | `14.14% <6.66%> (-0.29%)` | `4.00 <0.00> (ø)` | |
| [...rg/apache/gobblin/yarn/GobblinYarnAppLauncher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5BcHBMYXVuY2hlci5qYXZh) | `23.86% <8.33%> (-0.29%)` | `12.00 <0.00> (ø)` | |
| [...n/java/org/apache/gobblin/yarn/YarnHelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFybkhlbGl4VXRpbHMuamF2YQ==) | `32.75% <28.57%> (-1.94%)` | `9.00 <2.00> (+1.00)` | :arrow_down: |
| [.../apache/gobblin/yarn/GobblinApplicationMaster.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbkFwcGxpY2F0aW9uTWFzdGVyLmphdmE=) | `18.42% <100.00%> (+1.08%)` | `3.00 <0.00> (ø)` | |
| [.../org/apache/gobblin/util/filters/HiddenFilter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsdGVycy9IaWRkZW5GaWx0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| [...g/apache/gobblin/cluster/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...gobblin/runtime/mapreduce/GobblinOutputFormat.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbWFwcmVkdWNlL0dvYmJsaW5PdXRwdXRGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [.../gobblin/compaction/suite/CompactionSuiteBase.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc3VpdGUvQ29tcGFjdGlvblN1aXRlQmFzZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| [...obblin/compaction/source/CompactionFailedTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25GYWlsZWRUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| ... and [163 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?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/3207?src=pr&el=footer). Last update [2fa3553...9709cbd](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?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] ZihanLi58 commented on pull request #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#issuecomment-764879794
@sv2000 Can you help take a look at this change?
----------------------------------------------------------------
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 #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#discussion_r562157414
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java
##########
@@ -54,6 +55,7 @@
*
* @author Yinan Li
*/
+@Slf4j
Review comment:
Is this annotation needed? Looks like we are initializing a Logger already.
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java
##########
@@ -516,7 +518,13 @@ protected ContainerLaunchContext newContainerLaunchContext(Container container,
if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY)) {
addRemoteAppFiles(this.config.getString(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY), resourceMap);
}
-
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY)) {
+ addRemoteAppFiles(this.config.getString(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY), resourceMap);
+ }
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.APP_MASTER_ZIPS_REMOTE_KEY)) {
Review comment:
Should we have a separate config called CONTAINER_ZIPS_REMOTE_KEY?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -791,6 +798,20 @@ private void addAppRemoteFiles(String hdfsFileList, Map<String, LocalResource> r
}
}
+ private void addAppRemoteZips(String hdfsFileList, Map<String, LocalResource> resourceMap)
Review comment:
Can this method be moved to YarnHelixUtils to avoid code duplication in YarnService?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java
##########
@@ -516,7 +518,13 @@ protected ContainerLaunchContext newContainerLaunchContext(Container container,
if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY)) {
addRemoteAppFiles(this.config.getString(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY), resourceMap);
}
-
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY)) {
Review comment:
Lines 521-523 are duplicate of lines 518-520 and can be removed.
----------------------------------------------------------------
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 #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#discussion_r564880118
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -24,6 +24,7 @@
import java.nio.ByteBuffer;
import java.util.EnumSet;
import java.util.HashMap;
+import java.util.Iterator;
Review comment:
Unused import?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java
##########
@@ -124,4 +126,5 @@
public static final String GOBBLIN_YARN_AZKABAN_CLASS_LOG_LEVELS = GOBBLIN_YARN_PREFIX + "azkaban.class.logLevels";
//Container classpaths properties
public static final String GOBBLIN_YARN_ADDITIONAL_CLASSPATHS = GOBBLIN_YARN_PREFIX + "additional.classpaths";
+ public static final String GOBBLIN_YARN_CLASSPATHS = GOBBLIN_YARN_PREFIX + "classpaths";
Review comment:
Do we need an additional config? Or can we use gobblin.yarn.additional.classpaths?
----------------------------------------------------------------
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 #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#issuecomment-764446157
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=h1) Report
> Merging [#3207](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=desc) (9709cbd) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/2fa3553c75775d2d2a3a98f51c92b5c758c7abc9?el=desc) (2fa3553) will **decrease** coverage by `4.31%`.
> The diff coverage is `14.28%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3207 +/- ##
============================================
- Coverage 46.23% 41.92% -4.32%
+ Complexity 9788 8935 -853
============================================
Files 2019 2019
Lines 77439 77539 +100
Branches 8602 8609 +7
============================================
- Hits 35807 32506 -3301
- Misses 38300 41970 +3670
+ Partials 3332 3063 -269
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...che/gobblin/yarn/GobblinYarnConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `66.66% <ø> (ø)` | `1.00 <0.00> (ø)` | |
| [...main/java/org/apache/gobblin/yarn/YarnService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFyblNlcnZpY2UuamF2YQ==) | `14.14% <6.66%> (-0.29%)` | `4.00 <0.00> (ø)` | |
| [...rg/apache/gobblin/yarn/GobblinYarnAppLauncher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5BcHBMYXVuY2hlci5qYXZh) | `23.86% <8.33%> (-0.29%)` | `12.00 <0.00> (ø)` | |
| [...n/java/org/apache/gobblin/yarn/YarnHelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFybkhlbGl4VXRpbHMuamF2YQ==) | `32.75% <28.57%> (-1.94%)` | `9.00 <2.00> (+1.00)` | :arrow_down: |
| [.../apache/gobblin/yarn/GobblinApplicationMaster.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbkFwcGxpY2F0aW9uTWFzdGVyLmphdmE=) | `18.42% <100.00%> (+1.08%)` | `3.00 <0.00> (ø)` | |
| [.../org/apache/gobblin/util/filters/HiddenFilter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsdGVycy9IaWRkZW5GaWx0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| [...g/apache/gobblin/cluster/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...gobblin/runtime/mapreduce/GobblinOutputFormat.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbWFwcmVkdWNlL0dvYmJsaW5PdXRwdXRGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [.../gobblin/compaction/suite/CompactionSuiteBase.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc3VpdGUvQ29tcGFjdGlvblN1aXRlQmFzZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| [...obblin/compaction/source/CompactionFailedTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25GYWlsZWRUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| ... and [163 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?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/3207?src=pr&el=footer). Last update [2fa3553...9709cbd](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?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] codecov-io edited a comment on pull request #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#issuecomment-764446157
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=h1) Report
> Merging [#3207](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=desc) (481c0b2) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/2fa3553c75775d2d2a3a98f51c92b5c758c7abc9?el=desc) (2fa3553) will **decrease** coverage by `37.07%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3207 +/- ##
============================================
- Coverage 46.23% 9.16% -37.08%
+ Complexity 9788 1737 -8051
============================================
Files 2019 2019
Lines 77439 77534 +95
Branches 8602 8610 +8
============================================
- Hits 35807 7108 -28699
- Misses 38300 69731 +31431
+ Partials 3332 695 -2637
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../apache/gobblin/yarn/GobblinApplicationMaster.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbkFwcGxpY2F0aW9uTWFzdGVyLmphdmE=) | `0.00% <0.00%> (-17.34%)` | `0.00 <0.00> (-3.00)` | |
| [...rg/apache/gobblin/yarn/GobblinYarnAppLauncher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5BcHBMYXVuY2hlci5qYXZh) | `0.00% <0.00%> (-24.16%)` | `0.00 <0.00> (-12.00)` | |
| [...che/gobblin/yarn/GobblinYarnConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <ø> (-66.67%)` | `0.00 <0.00> (-1.00)` | |
| [...n/java/org/apache/gobblin/yarn/YarnHelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFybkhlbGl4VXRpbHMuamF2YQ==) | `0.00% <0.00%> (-34.70%)` | `0.00 <0.00> (-8.00)` | |
| [...main/java/org/apache/gobblin/yarn/YarnService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFyblNlcnZpY2UuamF2YQ==) | `0.00% <0.00%> (-14.44%)` | `0.00 <0.00> (-4.00)` | |
| [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvRmlsZVV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
| [...n/java/org/apache/gobblin/fork/CopyableSchema.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2ZvcmsvQ29weWFibGVTY2hlbWEuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [...java/org/apache/gobblin/stream/ControlMessage.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc3RyZWFtL0NvbnRyb2xNZXNzYWdlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...va/org/apache/gobblin/dataset/DatasetResolver.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YXNldC9EYXRhc2V0UmVzb2x2ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...va/org/apache/gobblin/converter/EmptyIterable.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9FbXB0eUl0ZXJhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| ... and [1070 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?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/3207?src=pr&el=footer). Last update [2fa3553...481c0b2](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?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] ZihanLi58 closed pull request #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
ZihanLi58 closed pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207
----------------------------------------------------------------
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 #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207
----------------------------------------------------------------
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] ZihanLi58 commented on pull request #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#issuecomment-764879794
@sv2000 Can you help take a look at this change?
----------------------------------------------------------------
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] ZihanLi58 commented on a change in pull request #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on a change in pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#discussion_r564882352
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java
##########
@@ -124,4 +126,5 @@
public static final String GOBBLIN_YARN_AZKABAN_CLASS_LOG_LEVELS = GOBBLIN_YARN_PREFIX + "azkaban.class.logLevels";
//Container classpaths properties
public static final String GOBBLIN_YARN_ADDITIONAL_CLASSPATHS = GOBBLIN_YARN_PREFIX + "additional.classpaths";
+ public static final String GOBBLIN_YARN_CLASSPATHS = GOBBLIN_YARN_PREFIX + "classpaths";
Review comment:
Additional path is to add new class path on the default one which is set the the yarn config. And in our case, we need to overwrite the default config to a different class path, so I just add a new config and keep the old one for backward compatibility and support more flexible use case.
----------------------------------------------------------------
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 #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#discussion_r562157414
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java
##########
@@ -54,6 +55,7 @@
*
* @author Yinan Li
*/
+@Slf4j
Review comment:
Is this annotation needed? Looks like we are initializing a Logger already.
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java
##########
@@ -516,7 +518,13 @@ protected ContainerLaunchContext newContainerLaunchContext(Container container,
if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY)) {
addRemoteAppFiles(this.config.getString(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY), resourceMap);
}
-
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY)) {
+ addRemoteAppFiles(this.config.getString(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY), resourceMap);
+ }
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.APP_MASTER_ZIPS_REMOTE_KEY)) {
Review comment:
Should we have a separate config called CONTAINER_ZIPS_REMOTE_KEY?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -791,6 +798,20 @@ private void addAppRemoteFiles(String hdfsFileList, Map<String, LocalResource> r
}
}
+ private void addAppRemoteZips(String hdfsFileList, Map<String, LocalResource> resourceMap)
Review comment:
Can this method be moved to YarnHelixUtils to avoid code duplication in YarnService?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java
##########
@@ -516,7 +518,13 @@ protected ContainerLaunchContext newContainerLaunchContext(Container container,
if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY)) {
addRemoteAppFiles(this.config.getString(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY), resourceMap);
}
-
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_REMOTE_KEY)) {
Review comment:
Lines 521-523 are duplicate of lines 518-520 and can be removed.
----------------------------------------------------------------
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 #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#issuecomment-764446157
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=h1) Report
> Merging [#3207](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=desc) (481c0b2) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/2fa3553c75775d2d2a3a98f51c92b5c758c7abc9?el=desc) (2fa3553) will **decrease** coverage by `0.04%`.
> The diff coverage is `19.35%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3207 +/- ##
============================================
- Coverage 46.23% 46.19% -0.05%
+ Complexity 9788 9774 -14
============================================
Files 2019 2019
Lines 77439 77534 +95
Branches 8602 8610 +8
============================================
+ Hits 35807 35817 +10
- Misses 38300 38387 +87
+ Partials 3332 3330 -2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...che/gobblin/yarn/GobblinYarnConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `66.66% <ø> (ø)` | `1.00 <0.00> (ø)` | |
| [...main/java/org/apache/gobblin/yarn/YarnService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFyblNlcnZpY2UuamF2YQ==) | `14.36% <0.00%> (-0.08%)` | `4.00 <0.00> (ø)` | |
| [...n/java/org/apache/gobblin/yarn/YarnHelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFybkhlbGl4VXRpbHMuamF2YQ==) | `26.66% <17.39%> (-8.03%)` | `9.00 <1.00> (+1.00)` | :arrow_down: |
| [...rg/apache/gobblin/yarn/GobblinYarnAppLauncher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5BcHBMYXVuY2hlci5qYXZh) | `24.36% <25.00%> (+0.21%)` | `12.00 <0.00> (ø)` | |
| [.../apache/gobblin/yarn/GobblinApplicationMaster.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbkFwcGxpY2F0aW9uTWFzdGVyLmphdmE=) | `18.42% <100.00%> (+1.08%)` | `3.00 <0.00> (ø)` | |
| [...lin/metrics/kafka/KafkaKeyValueProducerPusher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9tZXRyaWNzL2thZmthL0thZmthS2V5VmFsdWVQcm9kdWNlclB1c2hlci5qYXZh) | `0.00% <0.00%> (-82.76%)` | `0.00% <0.00%> (-7.00%)` | |
| [...che/gobblin/metrics/kafka/KafkaProducerPusher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9tZXRyaWNzL2thZmthL0thZmthUHJvZHVjZXJQdXNoZXIuamF2YQ==) | `0.00% <0.00%> (-82.15%)` | `0.00% <0.00%> (-7.00%)` | |
| [...he/gobblin/kafka/serialize/LiAvroDeserializer.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9rYWZrYS9zZXJpYWxpemUvTGlBdnJvRGVzZXJpYWxpemVyLmphdmE=) | `0.00% <0.00%> (-66.67%)` | `0.00% <0.00%> (-3.00%)` | |
| [...ache/gobblin/kafka/serialize/LiAvroSerializer.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9rYWZrYS9zZXJpYWxpemUvTGlBdnJvU2VyaWFsaXplci5qYXZh) | `0.00% <0.00%> (-50.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [.../apache/gobblin/kafka/writer/Kafka1DataWriter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9rYWZrYS93cml0ZXIvS2Fma2ExRGF0YVdyaXRlci5qYXZh) | `0.00% <0.00%> (-46.16%)` | `0.00% <0.00%> (-9.00%)` | |
| ... and [20 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3207/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?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/3207?src=pr&el=footer). Last update [2fa3553...481c0b2](https://codecov.io/gh/apache/incubator-gobblin/pull/3207?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] sv2000 commented on pull request #3207: [GOBBLIN-1386] Make gobblin yarn application be able to add zip fils as local resources, and make yarn class path to be configurable
Posted by GitBox <gi...@apache.org>.
sv2000 commented on pull request #3207:
URL: https://github.com/apache/incubator-gobblin/pull/3207#issuecomment-767898035
+1.
----------------------------------------------------------------
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