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/12/16 21:28:09 UTC
[GitHub] [incubator-gobblin] aplex opened a new pull request #3177: [GOBBLIN-1341] Make cluster name resolution more robust
aplex opened a new pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177
We extend cluster name conversion to handle names with
and without url schemes in config. In practice, scheme
and port numbers are irrelevant, as a single host usually
maps to a single cluster.
----------------------------------------------------------------
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 #3177: [GOBBLIN-1341] Make cluster name resolution more robust
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177#issuecomment-747076192
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=h1) Report
> Merging [#3177](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=desc) (7319d3e) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/0aad6d42b68caffadd0981559131d5eb8acae814?el=desc) (0aad6d4) will **decrease** coverage by `4.20%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3177 +/- ##
============================================
- Coverage 46.18% 41.98% -4.21%
+ Complexity 9719 8908 -811
============================================
Files 2006 2014 +8
Lines 76854 77112 +258
Branches 8547 8562 +15
============================================
- Hits 35498 32374 -3124
- Misses 38058 41691 +3633
+ Partials 3298 3047 -251
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...in/java/org/apache/gobblin/util/ClustersNames.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQ2x1c3RlcnNOYW1lcy5qYXZh) | `62.50% <100.00%> (+15.44%)` | `14.00 <10.00> (+5.00)` | |
| [.../org/apache/gobblin/util/filters/HiddenFilter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/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/3177/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/3177/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/3177/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/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25GYWlsZWRUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...n/cluster/event/ClusterManagerShutdownRequest.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvZXZlbnQvQ2x1c3Rlck1hbmFnZXJTaHV0ZG93blJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...blin/compaction/mapreduce/RecordKeyMapperBase.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vbWFwcmVkdWNlL1JlY29yZEtleU1hcHBlckJhc2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...in/compaction/action/CompactionCompleteAction.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vYWN0aW9uL0NvbXBhY3Rpb25Db21wbGV0ZUFjdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...n/compaction/mapreduce/orc/OrcKeyDedupReducer.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vbWFwcmVkdWNlL29yYy9PcmNLZXlEZWR1cFJlZHVjZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| ... and [159 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?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/3177?src=pr&el=footer). Last update [0aad6d4...7319d3e](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?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 #3177: [GOBBLIN-1341] Make cluster name resolution more robust
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177#issuecomment-747076192
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=h1) Report
> Merging [#3177](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=desc) (e93c773) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/0aad6d42b68caffadd0981559131d5eb8acae814?el=desc) (0aad6d4) will **decrease** coverage by `37.06%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3177 +/- ##
============================================
- Coverage 46.18% 9.12% -37.07%
+ Complexity 9719 1727 -7992
============================================
Files 2006 2014 +8
Lines 76854 77112 +258
Branches 8547 8562 +15
============================================
- Hits 35498 7035 -28463
- Misses 38058 69389 +31331
+ Partials 3298 688 -2610
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...in/java/org/apache/gobblin/util/ClustersNames.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQ2x1c3RlcnNOYW1lcy5qYXZh) | `23.43% <0.00%> (-23.63%)` | `3.00 <0.00> (-6.00)` | |
| [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/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/3177/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/3177/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/3177/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/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9FbXB0eUl0ZXJhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...org/apache/gobblin/ack/BasicAckableForTesting.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vYWNrL0Jhc2ljQWNrYWJsZUZvclRlc3RpbmcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...n/java/org/apache/gobblin/salesforce/SfConfig.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1zYWxlc2ZvcmNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NhbGVzZm9yY2UvU2ZDb25maWcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [.../org/apache/gobblin/yarn/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...va/org/apache/gobblin/cluster/SingleHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2luZ2xlSGVsaXhUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| ... and [1062 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?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/3177?src=pr&el=footer). Last update [0aad6d4...e93c773](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?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] autumnust commented on a change in pull request #3177: [GOBBLIN-1341] Make cluster name resolution more robust
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177#discussion_r545315929
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ClustersNames.java
##########
@@ -97,21 +110,34 @@ public void addClusterMapping(URL clusterUrl, String clusterName) {
this.urlToNameMap.put(clusterUrl.toString(), clusterName);
}
- // Strip out the port number if it is a valid URI
- private static String normalizeClusterUrl(String clusterIdentifier) {
+ private static List<String> generateUrlMatchCandidates(String clusterIdentifier) {
+ ArrayList<String> candidates = new ArrayList<>();
+ candidates.add(clusterIdentifier);
+
try {
URI uri = new URI(clusterIdentifier.trim());
- // URIs without protocol prefix
- if (!uri.isOpaque() && null != uri.getHost()) {
- clusterIdentifier = uri.getHost();
+ if (uri.getHost() != null) {
+ if (uri.getPort() != -1) {
+ candidates.add(uri.getHost() + ":" + uri.getPort());
+ }
+
+ candidates.add(uri.getHost());
+ } else if (uri.getScheme() != null && uri.getPath() != null) {
+ // we have a scheme and a path, but not the host name
+ // assuming local host
+ candidates.add("localhost");
} else {
- clusterIdentifier = uri.toString().replaceAll("[/:]"," ").trim().replaceAll(" ", "_");
+ candidates.add(getNormalizedName(clusterIdentifier));
}
} catch (URISyntaxException e) {
- //leave ID as is
+ candidates.add(getNormalizedName(clusterIdentifier));
}
- return clusterIdentifier;
+ return candidates;
+ }
+
+ private static String getNormalizedName(String clusterIdentifier) {
Review comment:
I am not quite following the purpose for this.
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ClustersNames.java
##########
@@ -97,21 +110,34 @@ public void addClusterMapping(URL clusterUrl, String clusterName) {
this.urlToNameMap.put(clusterUrl.toString(), clusterName);
}
- // Strip out the port number if it is a valid URI
- private static String normalizeClusterUrl(String clusterIdentifier) {
+ private static List<String> generateUrlMatchCandidates(String clusterIdentifier) {
+ ArrayList<String> candidates = new ArrayList<>();
+ candidates.add(clusterIdentifier);
+
try {
URI uri = new URI(clusterIdentifier.trim());
- // URIs without protocol prefix
- if (!uri.isOpaque() && null != uri.getHost()) {
- clusterIdentifier = uri.getHost();
+ if (uri.getHost() != null) {
+ if (uri.getPort() != -1) {
+ candidates.add(uri.getHost() + ":" + uri.getPort());
+ }
+
+ candidates.add(uri.getHost());
Review comment:
I don't see any chance for host:port to be picked here since getHost will always be placed after that. Is this intentional ? Or can you give an unit test case to return host:port
----------------------------------------------------------------
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 #3177: [GOBBLIN-1341] Make cluster name resolution more robust
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177#issuecomment-747076192
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=h1) Report
> Merging [#3177](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=desc) (e93c773) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/0aad6d42b68caffadd0981559131d5eb8acae814?el=desc) (0aad6d4) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3177 +/- ##
============================================
+ Coverage 46.18% 46.20% +0.01%
- Complexity 9719 9760 +41
============================================
Files 2006 2014 +8
Lines 76854 77112 +258
Branches 8547 8562 +15
============================================
+ Hits 35498 35631 +133
- Misses 38058 38173 +115
- Partials 3298 3308 +10
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...in/java/org/apache/gobblin/util/ClustersNames.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQ2x1c3RlcnNOYW1lcy5qYXZh) | `62.50% <100.00%> (+15.44%)` | `14.00 <10.00> (+5.00)` | |
| [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `85.71% <0.00%> (-7.15%)` | `3.00% <0.00%> (ø%)` | |
| [...source/extractor/hadoop/HadoopFileInputSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvaGFkb29wL0hhZG9vcEZpbGVJbnB1dFNvdXJjZS5qYXZh) | `59.32% <0.00%> (-4.32%)` | `6.00% <0.00%> (ø%)` | |
| [...kunit/packer/KafkaTopicGroupingWorkUnitPacker.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS93b3JrdW5pdC9wYWNrZXIvS2Fma2FUb3BpY0dyb3VwaW5nV29ya1VuaXRQYWNrZXIuamF2YQ==) | `39.49% <0.00%> (-0.68%)` | `11.00% <0.00%> (ø%)` | |
| [...e/gobblin/kafka/writer/KafkaDataWriterBuilder.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9rYWZrYS93cml0ZXIvS2Fma2FEYXRhV3JpdGVyQnVpbGRlci5qYXZh) | `0.00% <0.00%> (ø)` | `0.00% <0.00%> (?%)` | |
| [...che/gobblin/kafka/client/Kafka1ConsumerClient.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9rYWZrYS9jbGllbnQvS2Fma2ExQ29uc3VtZXJDbGllbnQuamF2YQ==) | `23.71% <0.00%> (ø)` | `6.00% <0.00%> (?%)` | |
| [...he/gobblin/kafka/serialize/LiAvroDeserializer.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9rYWZrYS9zZXJpYWxpemUvTGlBdnJvRGVzZXJpYWxpemVyLmphdmE=) | `66.66% <0.00%> (ø)` | `3.00% <0.00%> (?%)` | |
| [...che/gobblin/metrics/kafka/KafkaProducerPusher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9tZXRyaWNzL2thZmthL0thZmthUHJvZHVjZXJQdXNoZXIuamF2YQ==) | `82.14% <0.00%> (ø)` | `7.00% <0.00%> (?%)` | |
| [...in/kafka/writer/Kafka1JsonObjectWriterBuilder.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9rYWZrYS93cml0ZXIvS2Fma2ExSnNvbk9iamVjdFdyaXRlckJ1aWxkZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00% <0.00%> (?%)` | |
| [.../apache/gobblin/kafka/writer/Kafka1DataWriter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtMS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9rYWZrYS93cml0ZXIvS2Fma2ExRGF0YVdyaXRlci5qYXZh) | `46.15% <0.00%> (ø)` | `9.00% <0.00%> (?%)` | |
| ... and [4 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?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/3177?src=pr&el=footer). Last update [0aad6d4...e93c773](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?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] aplex commented on a change in pull request #3177: [GOBBLIN-1341] Make cluster name resolution more robust
Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177#discussion_r545349592
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ClustersNames.java
##########
@@ -97,21 +110,34 @@ public void addClusterMapping(URL clusterUrl, String clusterName) {
this.urlToNameMap.put(clusterUrl.toString(), clusterName);
}
- // Strip out the port number if it is a valid URI
- private static String normalizeClusterUrl(String clusterIdentifier) {
+ private static List<String> generateUrlMatchCandidates(String clusterIdentifier) {
+ ArrayList<String> candidates = new ArrayList<>();
+ candidates.add(clusterIdentifier);
+
try {
URI uri = new URI(clusterIdentifier.trim());
- // URIs without protocol prefix
- if (!uri.isOpaque() && null != uri.getHost()) {
- clusterIdentifier = uri.getHost();
+ if (uri.getHost() != null) {
+ if (uri.getPort() != -1) {
+ candidates.add(uri.getHost() + ":" + uri.getPort());
+ }
+
+ candidates.add(uri.getHost());
Review comment:
Added a comment here. Also check out "testPortSpecificOverrides" test
----------------------------------------------------------------
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 #3177: [GOBBLIN-1341] Make cluster name resolution more robust
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177#issuecomment-747076192
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=h1) Report
> Merging [#3177](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=desc) (06107e2) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/0aad6d42b68caffadd0981559131d5eb8acae814?el=desc) (0aad6d4) will **decrease** coverage by `37.08%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3177 +/- ##
============================================
- Coverage 46.18% 9.10% -37.09%
+ Complexity 9719 1724 -7995
============================================
Files 2006 2014 +8
Lines 76854 77108 +254
Branches 8547 8561 +14
============================================
- Hits 35498 7020 -28478
- Misses 38058 69403 +31345
+ Partials 3298 685 -2613
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...in/java/org/apache/gobblin/util/ClustersNames.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQ2x1c3RlcnNOYW1lcy5qYXZh) | `0.00% <0.00%> (-47.06%)` | `0.00 <0.00> (-9.00)` | |
| [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/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/3177/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/3177/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/3177/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/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9FbXB0eUl0ZXJhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...org/apache/gobblin/ack/BasicAckableForTesting.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vYWNrL0Jhc2ljQWNrYWJsZUZvclRlc3RpbmcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...n/java/org/apache/gobblin/salesforce/SfConfig.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1zYWxlc2ZvcmNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NhbGVzZm9yY2UvU2ZDb25maWcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [.../org/apache/gobblin/yarn/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...va/org/apache/gobblin/cluster/SingleHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2luZ2xlSGVsaXhUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| ... and [1062 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3177/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?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/3177?src=pr&el=footer). Last update [0aad6d4...06107e2](https://codecov.io/gh/apache/incubator-gobblin/pull/3177?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 #3177: [GOBBLIN-1341] Make cluster name resolution more robust
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177
----------------------------------------------------------------
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] aplex commented on a change in pull request #3177: [GOBBLIN-1341] Make cluster name resolution more robust
Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3177:
URL: https://github.com/apache/incubator-gobblin/pull/3177#discussion_r545350249
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ClustersNames.java
##########
@@ -97,21 +110,34 @@ public void addClusterMapping(URL clusterUrl, String clusterName) {
this.urlToNameMap.put(clusterUrl.toString(), clusterName);
}
- // Strip out the port number if it is a valid URI
- private static String normalizeClusterUrl(String clusterIdentifier) {
+ private static List<String> generateUrlMatchCandidates(String clusterIdentifier) {
+ ArrayList<String> candidates = new ArrayList<>();
+ candidates.add(clusterIdentifier);
+
try {
URI uri = new URI(clusterIdentifier.trim());
- // URIs without protocol prefix
- if (!uri.isOpaque() && null != uri.getHost()) {
- clusterIdentifier = uri.getHost();
+ if (uri.getHost() != null) {
+ if (uri.getPort() != -1) {
+ candidates.add(uri.getHost() + ":" + uri.getPort());
+ }
+
+ candidates.add(uri.getHost());
+ } else if (uri.getScheme() != null && uri.getPath() != null) {
+ // we have a scheme and a path, but not the host name
+ // assuming local host
+ candidates.add("localhost");
} else {
- clusterIdentifier = uri.toString().replaceAll("[/:]"," ").trim().replaceAll(" ", "_");
+ candidates.add(getNormalizedName(clusterIdentifier));
}
} catch (URISyntaxException e) {
- //leave ID as is
+ candidates.add(getNormalizedName(clusterIdentifier));
}
- return clusterIdentifier;
+ return candidates;
+ }
+
+ private static String getNormalizedName(String clusterIdentifier) {
Review comment:
Added logic explanation to getClusterName(String). Let me know if it is sufficiently clear there.
----------------------------------------------------------------
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