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/02/28 00:02:11 UTC
[GitHub] [gobblin] jhsenjaliya opened a new pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
jhsenjaliya opened a new pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236
…ive metastores
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] [gobblin] jhsenjaliya commented on a change in pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
jhsenjaliya commented on a change in pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#discussion_r646203963
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -771,20 +811,33 @@ private void addAppJars(String jarFilePathList, Optional<Map<String, LocalResour
}
}
- private void addAppLocalFiles(String localFilePathList, Optional<Map<String, LocalResource>> resourceMap,
- Path destDir, FileSystem localFs) throws IOException {
+ private void addAppLocalFiles(String localFilePath, String prefix, Path destDir,
+ Optional<Map<String, LocalResource>> resourceMap, FileSystem localFs) throws IOException {
+ try {
+ if(prefix == null) prefix = "";
- for (String localFilePath : SPLITTER.split(localFilePathList)) {
Path srcFilePath = new Path(localFilePath);
- Path destFilePath = new Path(destDir, srcFilePath.getName());
+ Path destFilePath = new Path(destDir, prefix + srcFilePath.getName());
if (localFs.exists(srcFilePath)) {
this.fs.copyFromLocalFile(srcFilePath, destFilePath);
if (resourceMap.isPresent()) {
YarnHelixUtils.addFileAsLocalResource(this.fs, destFilePath, LocalResourceType.FILE, resourceMap.get());
+ LOGGER.debug("The request file {} added as container local file for {}", srcFilePath, prefix);
}
} else {
- LOGGER.warn(String.format("The request file %s doesn't exist", srcFilePath));
+ LOGGER.warn("The request file {} doesn't exist for {}", srcFilePath, prefix);
}
+ }catch (IOException e){
+ LOGGER.error("Error copying app local files.", e);
+ throw e;
+ }
+ }
+
+ private void addAppLocalFiles(String localFilePathList, Optional<Map<String, LocalResource>> resourceMap,
Review comment:
just wanted to keep one for single file addition, but ya its mergeable into one. Thanks
--
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] [gobblin] autumnust commented on pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
autumnust commented on pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#issuecomment-791202791
Hi Jay, thanks for this patch. Starting to look at it.
Can you also update the your PR description for more context of what you changed as well as replace all Gobblin-XXX with the actual ticket number ? Thanks.
----------------------------------------------------------------
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] [gobblin] jhsenjaliya commented on a change in pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
jhsenjaliya commented on a change in pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#discussion_r646205907
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+ Map<String, ConfigObject> secureSystems = new HashMap<>();
+
+ if (allSysConfig == null) return secureSystems;
+
+ for (String systemName : allSysConfig.keySet()) {
+ ConfigObject sysConfig = allSysConfig.atPath(systemName).root();
+ sysConfig.get(IS_SECURE_KEY);
+ if (ConfigUtils.getBoolean(sysConfig.toConfig(), IS_SECURE_KEY, false)) {
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return multiple map of system name and its all config files regardless of the component.
+ */
+ public static Multimap<String, String> getAllSystemConfigFiles(Config config) {
+
+ Multimap<String, String> allSysConfFiles = LinkedHashMultimap.create();
+
+ getAllSystemConfig(config).entrySet();
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return allSysConfFiles;
+
+ for (Map.Entry<String, ConfigValue> sysConfig : allSysConfig.entrySet()) {
+
+ if (sysConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject allComponentConfigs = (ConfigObject) sysConfig.getValue();
+ for (Map.Entry<String, ConfigValue> componentConfig : allComponentConfigs.entrySet()) {
+ if (componentConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject componentConfigValue = (ConfigObject) componentConfig.getValue();
+ Config configValue = componentConfigValue.toConfig();
+ if (configValue.hasPath(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY)) {
+ configValue.getList(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY).forEach(item -> allSysConfFiles.put(sysConfig.getKey(), item.unwrapped().toString()));
+ }
+ }
+ }
+ }
+ }
+ return allSysConfFiles;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of all system -> config for which the token management {@link ConfigUtils#IS_TOKEN_MGMT_ENABLED_KEY} is enabled.
+ */
+ public static Map<String, Config> getAllSecureHiveSystems(Config config){
+ Map<String, Config> secureSystems = new HashMap<>();
+
+ for (String systemName : config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY).keySet()){
+ Config sysConfig = config.getConfig(GOBBLIN_SYNC_SYSTEMS_KEY).getConfig(systemName);
+ if(ConfigUtils.getBoolean(sysConfig, IS_SECURE_KEY, false) && ConfigUtils.getBoolean(sysConfig, IS_TOKEN_MGMT_ENABLED_KEY, false) && sysConfig.hasPath(HIVE_SYSTEM_KEY)){
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param metastoreURI for which we need to find system and its config
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name -> config paths for the given metastoreURI
+ */
+ public static Multimap<String, String> getSystemConfigForMetastore(String metastoreURI, Config config) {
+ if (config == null) {
+ config = getInstance();
+ }
+
+ Multimap<String, String> sysConfFiles = LinkedHashMultimap.create();
+
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return sysConfFiles;
+
+ for (Map.Entry<String, ConfigValue> sysConfig : allSysConfig.entrySet()) {
+
+ if (sysConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject allComponentConfigs = (ConfigObject) sysConfig.getValue();
Review comment:
this makes the traversing possible, only `ConfigObject` provides the entrySet method which `ConfigValue` does not. I think there can be other ways to obtain `ConfigObject` out of `ConfigValue` as well.
--
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] [gobblin] codecov-commenter edited a comment on pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#issuecomment-855515805
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3236](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (017777b) into [master](https://codecov.io/gh/apache/gobblin/commit/dd4c08d180b965960441c120ba649fb18ef21eef?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dd4c08d) will **decrease** coverage by `0.05%`.
> The diff coverage is `37.17%`.
> :exclamation: Current head 017777b differs from pull request most recent head 1449172. Consider uploading reports for the commit 1449172 to get more accurate results
[](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3236 +/- ##
============================================
- Coverage 46.48% 46.42% -0.06%
Complexity 10021 10021
============================================
Files 2037 2037
Lines 79292 79391 +99
Branches 8840 8866 +26
============================================
Hits 36855 36855
- Misses 39018 39115 +97
- Partials 3419 3421 +2
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <ø> (ø)` | |
| [...a/org/apache/gobblin/runtime/api/SpecExecutor.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcnVudGltZS9hcGkvU3BlY0V4ZWN1dG9yLmphdmE=) | `0.00% <ø> (ø)` | |
| [...a/org/apache/gobblin/runtime/api/SpecProducer.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcnVudGltZS9hcGkvU3BlY1Byb2R1Y2VyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../apache/gobblin/cluster/GobblinClusterManager.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkNsdXN0ZXJNYW5hZ2VyLmphdmE=) | `52.09% <ø> (ø)` | |
| [...rg/apache/gobblin/cluster/GobblinClusterUtils.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkNsdXN0ZXJVdGlscy5qYXZh) | `90.74% <ø> (ø)` | |
| [...ata/management/copy/hive/HivePartitionFileSet.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvaGl2ZS9IaXZlUGFydGl0aW9uRmlsZVNldC5qYXZh) | `0.93% <0.00%> (ø)` | |
| [...nt/copy/writer/FileAwareInputStreamDataWriter.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvd3JpdGVyL0ZpbGVBd2FyZUlucHV0U3RyZWFtRGF0YVdyaXRlci5qYXZh) | `74.37% <ø> (ø)` | |
| [...ain/java/org/apache/gobblin/hive/HiveRegister.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL0hpdmVSZWdpc3Rlci5qYXZh) | `14.10% <ø> (ø)` | |
| [...lin/hive/metastore/HiveMetaStoreBasedRegister.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL21ldGFzdG9yZS9IaXZlTWV0YVN0b3JlQmFzZWRSZWdpc3Rlci5qYXZh) | `10.24% <0.00%> (ø)` | |
| [...apache/gobblin/hive/writer/HiveMetadataWriter.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL3dyaXRlci9IaXZlTWV0YWRhdGFXcml0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| ... and [132 more](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [dd4c08d...1449172](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] [gobblin] jhsenjaliya commented on a change in pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
jhsenjaliya commented on a change in pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#discussion_r604358093
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -34,6 +34,9 @@
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
+
+import com.google.common.collect.*;
Review comment:
ya, need to setup my new intelliJ configs, will update.
--
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] [gobblin] jhsenjaliya commented on pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
jhsenjaliya commented on pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#issuecomment-855510149
@autumnust , addressed all comments, can u pls take a look?
about the testcase, we dont have much structure in place for it, but I will take a look at `GobblinYarnAppLauncherTest` and see if we can add it. Thanks
--
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] [gobblin] codecov-commenter commented on pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#issuecomment-855515805
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3236](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (017777b) into [master](https://codecov.io/gh/apache/gobblin/commit/dd4c08d180b965960441c120ba649fb18ef21eef?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dd4c08d) will **decrease** coverage by `37.49%`.
> The diff coverage is `3.84%`.
> :exclamation: Current head 017777b differs from pull request most recent head 1449172. Consider uploading reports for the commit 1449172 to get more accurate results
[](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3236 +/- ##
============================================
- Coverage 46.48% 8.98% -37.50%
+ Complexity 10021 1742 -8279
============================================
Files 2037 2037
Lines 79292 79391 +99
Branches 8840 8866 +26
============================================
- Hits 36855 7135 -29720
- Misses 39018 71554 +32536
+ Partials 3419 702 -2717
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <ø> (ø)` | |
| [...bblin/password/EncryptedPasswordAuthenticator.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcGFzc3dvcmQvRW5jcnlwdGVkUGFzc3dvcmRBdXRoZW50aWNhdG9yLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/gobblin/runtime/api/SpecExecutor.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcnVudGltZS9hcGkvU3BlY0V4ZWN1dG9yLmphdmE=) | `0.00% <ø> (ø)` | |
| [...a/org/apache/gobblin/runtime/api/SpecProducer.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcnVudGltZS9hcGkvU3BlY1Byb2R1Y2VyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../apache/gobblin/cluster/GobblinClusterManager.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkNsdXN0ZXJNYW5hZ2VyLmphdmE=) | `42.79% <ø> (-9.31%)` | :arrow_down: |
| [...rg/apache/gobblin/cluster/GobblinClusterUtils.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkNsdXN0ZXJVdGlscy5qYXZh) | `35.18% <ø> (-55.56%)` | :arrow_down: |
| [...paction/action/CompactionGMCEPublishingAction.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vYWN0aW9uL0NvbXBhY3Rpb25HTUNFUHVibGlzaGluZ0FjdGlvbi5qYXZh) | `0.00% <0.00%> (-56.53%)` | :arrow_down: |
| [...actionSuiteBaseWithConfigurableCompleteAction.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc3VpdGUvQ29tcGFjdGlvblN1aXRlQmFzZVdpdGhDb25maWd1cmFibGVDb21wbGV0ZUFjdGlvbi5qYXZh) | `0.00% <0.00%> (-83.34%)` | :arrow_down: |
| [.../destination/DestinationDatasetHandlerFactory.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2Rlc3RpbmF0aW9uL0Rlc3RpbmF0aW9uRGF0YXNldEhhbmRsZXJGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (-57.15%)` | :arrow_down: |
| [.../destination/DestinationDatasetHandlerService.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2Rlc3RpbmF0aW9uL0Rlc3RpbmF0aW9uRGF0YXNldEhhbmRsZXJTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (-66.67%)` | :arrow_down: |
| ... and [1172 more](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [dd4c08d...1449172](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] [gobblin] codecov-commenter edited a comment on pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#issuecomment-855515805
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3236](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1449172) into [master](https://codecov.io/gh/apache/gobblin/commit/dd4c08d180b965960441c120ba649fb18ef21eef?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dd4c08d) will **decrease** coverage by `0.07%`.
> The diff coverage is `3.77%`.
[](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3236 +/- ##
============================================
- Coverage 46.48% 46.40% -0.08%
+ Complexity 10021 10020 -1
============================================
Files 2037 2037
Lines 79292 79391 +99
Branches 8840 8866 +26
============================================
- Hits 36855 36845 -10
- Misses 39018 39123 +105
- Partials 3419 3423 +4
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...main/java/org/apache/gobblin/util/ConfigUtils.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQ29uZmlnVXRpbHMuamF2YQ==) | `40.96% <0.00%> (-17.90%)` | :arrow_down: |
| [...rg/apache/gobblin/yarn/GobblinYarnAppLauncher.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5BcHBMYXVuY2hlci5qYXZh) | `23.37% <0.00%> (-0.94%)` | :arrow_down: |
| [...che/gobblin/yarn/GobblinYarnConfigurationKeys.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `66.66% <ø> (ø)` | |
| [.../java/org/apache/gobblin/hive/HiveConfFactory.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL0hpdmVDb25mRmFjdG9yeS5qYXZh) | `57.69% <36.36%> (-15.65%)` | :arrow_down: |
| [...he/gobblin/source/PartitionAwareFileRetriever.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9QYXJ0aXRpb25Bd2FyZUZpbGVSZXRyaWV2ZXIuamF2YQ==) | `48.14% <0.00%> (-7.41%)` | :arrow_down: |
| [...g/apache/gobblin/hive/orc/HiveOrcSerDeManager.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL29yYy9IaXZlT3JjU2VyRGVNYW5hZ2VyLmphdmE=) | `60.46% <0.00%> (-3.49%)` | :arrow_down: |
| [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `34.71% <0.00%> (-3.31%)` | :arrow_down: |
| [...lin/restli/throttling/ZookeeperLeaderElection.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2UvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2Utc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3Jlc3RsaS90aHJvdHRsaW5nL1pvb2tlZXBlckxlYWRlckVsZWN0aW9uLmphdmE=) | `70.00% <0.00%> (-2.23%)` | :arrow_down: |
| [...a/management/copy/publisher/CopyDataPublisher.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvcHVibGlzaGVyL0NvcHlEYXRhUHVibGlzaGVyLmphdmE=) | `74.17% <0.00%> (-1.33%)` | :arrow_down: |
| [...pache/gobblin/runtime/GobblinMultiTaskAttempt.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvR29iYmxpbk11bHRpVGFza0F0dGVtcHQuamF2YQ==) | `62.30% <0.00%> (-0.40%)` | :arrow_down: |
| ... and [3 more](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [dd4c08d...1449172](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] [gobblin] jhsenjaliya commented on a change in pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
jhsenjaliya commented on a change in pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#discussion_r646204591
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -67,12 +70,48 @@
*/
private static final String GOBBLIN_CONFIG_BLACKLIST_KEYS = "gobblin.config.blacklistKeys";
+ /**
+ * Gobblin sync system specific config
+ */
+ public static final String GOBBLIN_SYNC_SYSTEMS_KEY = "gobblin_sync_systems";
+ public static final String GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY = "config_files";
+ public static final String GOBBLIN_SYNC_SYSTEM_METASTORE_URI_KEY = "metastore.uris";
+ public static final String IS_SECURE_KEY = "is_secure";
+ public static final String IS_TOKEN_MGMT_ENABLED_KEY = "is_token_management_enabled";
+ public static final String CONFIG_FILES_LIST_KEY = "config_files";
+ public static final String HIVE_SYSTEM_KEY = "hive";
+ public static final String HIVE_METASTORE_URI_KEY = "metastore.uris";
+
+
/**
* A suffix that is automatically appended to property keys that are prefixes of other
* property keys. This is used during Properties -> Config -> Properties conversion since
* typesafe config does not allow such properties. */
public static final String STRIP_SUFFIX = ".ROOT_VALUE";
+ private static volatile Config instance;
+ public static Config getInstance() {
Review comment:
the empty wont have the config from application.conf and i notice we keep loading config many times in code. so this method should set the base for all initial config and provide the singleton instead of loading the config multiple times.
--
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] [gobblin] codecov-io commented on pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#issuecomment-787209220
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=h1) Report
> Merging [#3236](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=desc) (6b2a3ee) into [master](https://codecov.io/gh/apache/gobblin/commit/4fb83cccec15739d4afe54aa288666ef81852156?el=desc) (4fb83cc) will **decrease** coverage by `37.35%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3236 +/- ##
============================================
- Coverage 46.37% 9.02% -37.36%
+ Complexity 9935 1738 -8197
============================================
Files 2030 2030
Lines 78744 78849 +105
Branches 8759 8783 +24
============================================
- Hits 36520 7117 -29403
- Misses 38822 71036 +32214
+ Partials 3402 696 -2706
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3236?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../java/org/apache/gobblin/hive/HiveConfFactory.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL0hpdmVDb25mRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-73.34%)` | `0.00 <0.00> (-7.00)` | |
| [...main/java/org/apache/gobblin/util/ConfigUtils.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQ29uZmlnVXRpbHMuamF2YQ==) | `13.00% <0.00%> (-45.86%)` | `11.00 <0.00> (-30.00)` | |
| [...rg/apache/gobblin/yarn/GobblinYarnAppLauncher.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5BcHBMYXVuY2hlci5qYXZh) | `0.00% <0.00%> (-24.32%)` | `0.00 <0.00> (-12.00)` | |
| [...che/gobblin/yarn/GobblinYarnConfigurationKeys.java](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <ø> (-66.67%)` | `0.00 <0.00> (-1.00)` | |
| [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/gobblin/pull/3236/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/gobblin/pull/3236/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/gobblin/pull/3236/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/gobblin/pull/3236/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/gobblin/pull/3236/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/gobblin/pull/3236/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vYWNrL0Jhc2ljQWNrYWJsZUZvclRlc3RpbmcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| ... and [1073 more](https://codecov.io/gh/apache/gobblin/pull/3236/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3236?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/gobblin/pull/3236?src=pr&el=footer). Last update [4fb83cc...6b2a3ee](https://codecov.io/gh/apache/gobblin/pull/3236?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] [gobblin] autumnust commented on a change in pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#discussion_r598033242
##########
File path: gobblin-hive-registration/src/main/java/org/apache/gobblin/hive/HiveConfFactory.java
##########
@@ -18,8 +18,13 @@
package org.apache.gobblin.hive;
import java.io.IOException;
+import java.util.Map;
+import com.google.common.collect.Multimap;
Review comment:
Are you using IntelliJ ? You could use the style xml file that is available here :https://gobblin.readthedocs.io/en/latest/developer-guide/CodingStyle/ to help auto-formatting. Looks like the import order isn't correct here. I also notice some lines in the PR being too long, trailing space, etc.
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -67,12 +70,48 @@
*/
private static final String GOBBLIN_CONFIG_BLACKLIST_KEYS = "gobblin.config.blacklistKeys";
+ /**
+ * Gobblin sync system specific config
+ */
+ public static final String GOBBLIN_SYNC_SYSTEMS_KEY = "gobblin_sync_systems";
+ public static final String GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY = "config_files";
+ public static final String GOBBLIN_SYNC_SYSTEM_METASTORE_URI_KEY = "metastore.uris";
+ public static final String IS_SECURE_KEY = "is_secure";
+ public static final String IS_TOKEN_MGMT_ENABLED_KEY = "is_token_management_enabled";
+ public static final String CONFIG_FILES_LIST_KEY = "config_files";
+ public static final String HIVE_SYSTEM_KEY = "hive";
+ public static final String HIVE_METASTORE_URI_KEY = "metastore.uris";
+
+
/**
* A suffix that is automatically appended to property keys that are prefixes of other
* property keys. This is used during Properties -> Config -> Properties conversion since
* typesafe config does not allow such properties. */
public static final String STRIP_SUFFIX = ".ROOT_VALUE";
+ private static volatile Config instance;
+ public static Config getInstance() {
Review comment:
I believe this method is not necessary since it do the same thing as `ConfigFactory.empty()`
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java
##########
@@ -65,6 +65,7 @@
public static final String CONTAINER_JARS_KEY = GOBBLIN_YARN_PREFIX + "container.jars";
public static final String CONTAINER_FILES_LOCAL_KEY = GOBBLIN_YARN_PREFIX + "container.files.local";
public static final String CONTAINER_FILES_REMOTE_KEY = GOBBLIN_YARN_PREFIX + "container.files.remote";
+ public static final String CONTAINER_COPY_SYSTEM_CONFIGS = GOBBLIN_YARN_PREFIX + "container.copy_sync_system_configs";
Review comment:
it should be named as camelCase instead of connecting different phrases by underscore ( as a usual java convention
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
Review comment:
there's an additional blank before the ending parentheses
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -34,6 +34,9 @@
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
+
+import com.google.common.collect.*;
Review comment:
It is not recommended to import star.
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -771,20 +811,33 @@ private void addAppJars(String jarFilePathList, Optional<Map<String, LocalResour
}
}
- private void addAppLocalFiles(String localFilePathList, Optional<Map<String, LocalResource>> resourceMap,
- Path destDir, FileSystem localFs) throws IOException {
+ private void addAppLocalFiles(String localFilePath, String prefix, Path destDir,
+ Optional<Map<String, LocalResource>> resourceMap, FileSystem localFs) throws IOException {
+ try {
+ if(prefix == null) prefix = "";
- for (String localFilePath : SPLITTER.split(localFilePathList)) {
Path srcFilePath = new Path(localFilePath);
- Path destFilePath = new Path(destDir, srcFilePath.getName());
+ Path destFilePath = new Path(destDir, prefix + srcFilePath.getName());
if (localFs.exists(srcFilePath)) {
this.fs.copyFromLocalFile(srcFilePath, destFilePath);
if (resourceMap.isPresent()) {
YarnHelixUtils.addFileAsLocalResource(this.fs, destFilePath, LocalResourceType.FILE, resourceMap.get());
+ LOGGER.debug("The request file {} added as container local file for {}", srcFilePath, prefix);
}
} else {
- LOGGER.warn(String.format("The request file %s doesn't exist", srcFilePath));
+ LOGGER.warn("The request file {} doesn't exist for {}", srcFilePath, prefix);
}
+ }catch (IOException e){
+ LOGGER.error("Error copying app local files.", e);
+ throw e;
+ }
+ }
+
+ private void addAppLocalFiles(String localFilePathList, Optional<Map<String, LocalResource>> resourceMap,
Review comment:
why do we need a separate method if changing the semantics of `localFilePath` in the `addAppLocalFiles` to be either string to comma-separated-string is what's really needed?
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+ Map<String, ConfigObject> secureSystems = new HashMap<>();
+
+ if (allSysConfig == null) return secureSystems;
+
+ for (String systemName : allSysConfig.keySet()) {
+ ConfigObject sysConfig = allSysConfig.atPath(systemName).root();
+ sysConfig.get(IS_SECURE_KEY);
+ if (ConfigUtils.getBoolean(sysConfig.toConfig(), IS_SECURE_KEY, false)) {
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return multiple map of system name and its all config files regardless of the component.
+ */
+ public static Multimap<String, String> getAllSystemConfigFiles(Config config) {
+
+ Multimap<String, String> allSysConfFiles = LinkedHashMultimap.create();
+
+ getAllSystemConfig(config).entrySet();
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return allSysConfFiles;
Review comment:
I think as a general java best practice it is always recommended to use curly brackets even there's only one line
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+ Map<String, ConfigObject> secureSystems = new HashMap<>();
+
+ if (allSysConfig == null) return secureSystems;
+
+ for (String systemName : allSysConfig.keySet()) {
+ ConfigObject sysConfig = allSysConfig.atPath(systemName).root();
+ sysConfig.get(IS_SECURE_KEY);
Review comment:
what's the purpose of this line?
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+ Map<String, ConfigObject> secureSystems = new HashMap<>();
+
+ if (allSysConfig == null) return secureSystems;
+
+ for (String systemName : allSysConfig.keySet()) {
+ ConfigObject sysConfig = allSysConfig.atPath(systemName).root();
+ sysConfig.get(IS_SECURE_KEY);
+ if (ConfigUtils.getBoolean(sysConfig.toConfig(), IS_SECURE_KEY, false)) {
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return multiple map of system name and its all config files regardless of the component.
+ */
+ public static Multimap<String, String> getAllSystemConfigFiles(Config config) {
+
+ Multimap<String, String> allSysConfFiles = LinkedHashMultimap.create();
+
+ getAllSystemConfig(config).entrySet();
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return allSysConfFiles;
+
+ for (Map.Entry<String, ConfigValue> sysConfig : allSysConfig.entrySet()) {
+
+ if (sysConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject allComponentConfigs = (ConfigObject) sysConfig.getValue();
+ for (Map.Entry<String, ConfigValue> componentConfig : allComponentConfigs.entrySet()) {
+ if (componentConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject componentConfigValue = (ConfigObject) componentConfig.getValue();
+ Config configValue = componentConfigValue.toConfig();
+ if (configValue.hasPath(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY)) {
+ configValue.getList(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY).forEach(item -> allSysConfFiles.put(sysConfig.getKey(), item.unwrapped().toString()));
+ }
+ }
+ }
+ }
+ }
+ return allSysConfFiles;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of all system -> config for which the token management {@link ConfigUtils#IS_TOKEN_MGMT_ENABLED_KEY} is enabled.
+ */
+ public static Map<String, Config> getAllSecureHiveSystems(Config config){
+ Map<String, Config> secureSystems = new HashMap<>();
+
+ for (String systemName : config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY).keySet()){
+ Config sysConfig = config.getConfig(GOBBLIN_SYNC_SYSTEMS_KEY).getConfig(systemName);
+ if(ConfigUtils.getBoolean(sysConfig, IS_SECURE_KEY, false) && ConfigUtils.getBoolean(sysConfig, IS_TOKEN_MGMT_ENABLED_KEY, false) && sysConfig.hasPath(HIVE_SYSTEM_KEY)){
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param metastoreURI for which we need to find system and its config
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name -> config paths for the given metastoreURI
+ */
+ public static Multimap<String, String> getSystemConfigForMetastore(String metastoreURI, Config config) {
+ if (config == null) {
+ config = getInstance();
+ }
+
+ Multimap<String, String> sysConfFiles = LinkedHashMultimap.create();
+
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return sysConfFiles;
+
+ for (Map.Entry<String, ConfigValue> sysConfig : allSysConfig.entrySet()) {
+
+ if (sysConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject allComponentConfigs = (ConfigObject) sysConfig.getValue();
+ for (Map.Entry<String, ConfigValue> componentConfig : allComponentConfigs.entrySet()) {
+ if (componentConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject componentConfigValue = (ConfigObject) componentConfig.getValue();
+ Config configValue = componentConfigValue.toConfig();
+ if (configValue.hasPath(GOBBLIN_SYNC_SYSTEM_METASTORE_URI_KEY)) {
Review comment:
this seems to be unnecessary to have 3-layer if branch here.
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -67,12 +70,48 @@
*/
private static final String GOBBLIN_CONFIG_BLACKLIST_KEYS = "gobblin.config.blacklistKeys";
+ /**
+ * Gobblin sync system specific config
+ */
+ public static final String GOBBLIN_SYNC_SYSTEMS_KEY = "gobblin_sync_systems";
+ public static final String GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY = "config_files";
+ public static final String GOBBLIN_SYNC_SYSTEM_METASTORE_URI_KEY = "metastore.uris";
+ public static final String IS_SECURE_KEY = "is_secure";
+ public static final String IS_TOKEN_MGMT_ENABLED_KEY = "is_token_management_enabled";
+ public static final String CONFIG_FILES_LIST_KEY = "config_files";
+ public static final String HIVE_SYSTEM_KEY = "hive";
+ public static final String HIVE_METASTORE_URI_KEY = "metastore.uris";
+
+
/**
* A suffix that is automatically appended to property keys that are prefixes of other
* property keys. This is used during Properties -> Config -> Properties conversion since
* typesafe config does not allow such properties. */
public static final String STRIP_SUFFIX = ".ROOT_VALUE";
+ private static volatile Config instance;
+ public static Config getInstance() {
+ if (instance == null) {
+ synchronized (Config.class) {
+ if (instance == null) {
+ instance = ConfigFactory.load();
+ }
+ }
+ }
+ return instance;
+ }
+
+ public static Config getInstance(boolean notFromCache) {
Review comment:
Same as above
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+ Map<String, ConfigObject> secureSystems = new HashMap<>();
+
+ if (allSysConfig == null) return secureSystems;
+
+ for (String systemName : allSysConfig.keySet()) {
+ ConfigObject sysConfig = allSysConfig.atPath(systemName).root();
+ sysConfig.get(IS_SECURE_KEY);
+ if (ConfigUtils.getBoolean(sysConfig.toConfig(), IS_SECURE_KEY, false)) {
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return multiple map of system name and its all config files regardless of the component.
+ */
+ public static Multimap<String, String> getAllSystemConfigFiles(Config config) {
+
+ Multimap<String, String> allSysConfFiles = LinkedHashMultimap.create();
+
+ getAllSystemConfig(config).entrySet();
Review comment:
result is not being captured by any object, is this intentional ?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -771,20 +811,33 @@ private void addAppJars(String jarFilePathList, Optional<Map<String, LocalResour
}
}
- private void addAppLocalFiles(String localFilePathList, Optional<Map<String, LocalResource>> resourceMap,
- Path destDir, FileSystem localFs) throws IOException {
+ private void addAppLocalFiles(String localFilePath, String prefix, Path destDir,
+ Optional<Map<String, LocalResource>> resourceMap, FileSystem localFs) throws IOException {
+ try {
+ if(prefix == null) prefix = "";
- for (String localFilePath : SPLITTER.split(localFilePathList)) {
Path srcFilePath = new Path(localFilePath);
- Path destFilePath = new Path(destDir, srcFilePath.getName());
+ Path destFilePath = new Path(destDir, prefix + srcFilePath.getName());
if (localFs.exists(srcFilePath)) {
this.fs.copyFromLocalFile(srcFilePath, destFilePath);
if (resourceMap.isPresent()) {
YarnHelixUtils.addFileAsLocalResource(this.fs, destFilePath, LocalResourceType.FILE, resourceMap.get());
+ LOGGER.debug("The request file {} added as container local file for {}", srcFilePath, prefix);
}
} else {
- LOGGER.warn(String.format("The request file %s doesn't exist", srcFilePath));
+ LOGGER.warn("The request file {} doesn't exist for {}", srcFilePath, prefix);
}
+ }catch (IOException e){
+ LOGGER.error("Error copying app local files.", e);
Review comment:
it is not recommended to log the error and the throw the exception including the same error object, or it causes confusion when looking at the stacktrace.
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+ Map<String, ConfigObject> secureSystems = new HashMap<>();
+
+ if (allSysConfig == null) return secureSystems;
+
+ for (String systemName : allSysConfig.keySet()) {
+ ConfigObject sysConfig = allSysConfig.atPath(systemName).root();
+ sysConfig.get(IS_SECURE_KEY);
+ if (ConfigUtils.getBoolean(sysConfig.toConfig(), IS_SECURE_KEY, false)) {
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return multiple map of system name and its all config files regardless of the component.
+ */
+ public static Multimap<String, String> getAllSystemConfigFiles(Config config) {
+
+ Multimap<String, String> allSysConfFiles = LinkedHashMultimap.create();
+
+ getAllSystemConfig(config).entrySet();
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return allSysConfFiles;
+
+ for (Map.Entry<String, ConfigValue> sysConfig : allSysConfig.entrySet()) {
+
+ if (sysConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject allComponentConfigs = (ConfigObject) sysConfig.getValue();
+ for (Map.Entry<String, ConfigValue> componentConfig : allComponentConfigs.entrySet()) {
+ if (componentConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject componentConfigValue = (ConfigObject) componentConfig.getValue();
+ Config configValue = componentConfigValue.toConfig();
+ if (configValue.hasPath(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY)) {
+ configValue.getList(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY).forEach(item -> allSysConfFiles.put(sysConfig.getKey(), item.unwrapped().toString()));
+ }
+ }
+ }
+ }
+ }
+ return allSysConfFiles;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of all system -> config for which the token management {@link ConfigUtils#IS_TOKEN_MGMT_ENABLED_KEY} is enabled.
+ */
+ public static Map<String, Config> getAllSecureHiveSystems(Config config){
+ Map<String, Config> secureSystems = new HashMap<>();
+
+ for (String systemName : config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY).keySet()){
+ Config sysConfig = config.getConfig(GOBBLIN_SYNC_SYSTEMS_KEY).getConfig(systemName);
+ if(ConfigUtils.getBoolean(sysConfig, IS_SECURE_KEY, false) && ConfigUtils.getBoolean(sysConfig, IS_TOKEN_MGMT_ENABLED_KEY, false) && sysConfig.hasPath(HIVE_SYSTEM_KEY)){
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param metastoreURI for which we need to find system and its config
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name -> config paths for the given metastoreURI
+ */
+ public static Multimap<String, String> getSystemConfigForMetastore(String metastoreURI, Config config) {
+ if (config == null) {
+ config = getInstance();
+ }
+
+ Multimap<String, String> sysConfFiles = LinkedHashMultimap.create();
+
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return sysConfFiles;
Review comment:
the same curly bracket comment.
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+ Map<String, ConfigObject> secureSystems = new HashMap<>();
+
+ if (allSysConfig == null) return secureSystems;
+
+ for (String systemName : allSysConfig.keySet()) {
+ ConfigObject sysConfig = allSysConfig.atPath(systemName).root();
+ sysConfig.get(IS_SECURE_KEY);
+ if (ConfigUtils.getBoolean(sysConfig.toConfig(), IS_SECURE_KEY, false)) {
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return multiple map of system name and its all config files regardless of the component.
+ */
+ public static Multimap<String, String> getAllSystemConfigFiles(Config config) {
+
+ Multimap<String, String> allSysConfFiles = LinkedHashMultimap.create();
+
+ getAllSystemConfig(config).entrySet();
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return allSysConfFiles;
+
+ for (Map.Entry<String, ConfigValue> sysConfig : allSysConfig.entrySet()) {
+
+ if (sysConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject allComponentConfigs = (ConfigObject) sysConfig.getValue();
+ for (Map.Entry<String, ConfigValue> componentConfig : allComponentConfigs.entrySet()) {
+ if (componentConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject componentConfigValue = (ConfigObject) componentConfig.getValue();
+ Config configValue = componentConfigValue.toConfig();
+ if (configValue.hasPath(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY)) {
+ configValue.getList(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY).forEach(item -> allSysConfFiles.put(sysConfig.getKey(), item.unwrapped().toString()));
+ }
+ }
+ }
+ }
+ }
+ return allSysConfFiles;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of all system -> config for which the token management {@link ConfigUtils#IS_TOKEN_MGMT_ENABLED_KEY} is enabled.
+ */
+ public static Map<String, Config> getAllSecureHiveSystems(Config config){
+ Map<String, Config> secureSystems = new HashMap<>();
+
+ for (String systemName : config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY).keySet()){
+ Config sysConfig = config.getConfig(GOBBLIN_SYNC_SYSTEMS_KEY).getConfig(systemName);
+ if(ConfigUtils.getBoolean(sysConfig, IS_SECURE_KEY, false) && ConfigUtils.getBoolean(sysConfig, IS_TOKEN_MGMT_ENABLED_KEY, false) && sysConfig.hasPath(HIVE_SYSTEM_KEY)){
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param metastoreURI for which we need to find system and its config
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name -> config paths for the given metastoreURI
+ */
+ public static Multimap<String, String> getSystemConfigForMetastore(String metastoreURI, Config config) {
+ if (config == null) {
+ config = getInstance();
+ }
+
+ Multimap<String, String> sysConfFiles = LinkedHashMultimap.create();
+
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return sysConfFiles;
+
+ for (Map.Entry<String, ConfigValue> sysConfig : allSysConfig.entrySet()) {
+
+ if (sysConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject allComponentConfigs = (ConfigObject) sysConfig.getValue();
Review comment:
Why this cast is needed? Is there any better way to traverse the typesafe config object?
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
Review comment:
1. This method seems to just filtering based on the value of `IS_SECURE_KEY` for an config object. Can we make it simpler ?
2. This method seems to be very specific to a use case, which is about whether the "system" is "secure". It doesn't seem to be a good fit for `ConfigUtils` as the utility class shouldn't be exposed to anything about the config contents itself.
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -771,20 +811,33 @@ private void addAppJars(String jarFilePathList, Optional<Map<String, LocalResour
}
}
- private void addAppLocalFiles(String localFilePathList, Optional<Map<String, LocalResource>> resourceMap,
- Path destDir, FileSystem localFs) throws IOException {
+ private void addAppLocalFiles(String localFilePath, String prefix, Path destDir,
Review comment:
What does `prefix` mean here? Can you add javadoc to explain why this is needed?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -710,6 +711,26 @@ private Resource prepareContainerResource(GetNewApplicationResponse newApplicati
appMasterResources);
}
+ // handles all system configs files automatically, so it does not has to be mentioned explicitly as values of CONTAINER_FILES_LOCAL_KEY
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_COPY_SYSTEM_CONFIGS) && this.config.getBoolean(GobblinYarnConfigurationKeys.CONTAINER_COPY_SYSTEM_CONFIGS)) {
+ Path appFilesDestDir = new Path(appMasterWorkDir, GobblinYarnConfigurationKeys.APP_FILES_DIR_NAME);
+
+ Multimap<String, String> allSysConfigFiles = ConfigUtils.getAllSystemConfigFiles(this.config);
+ allSysConfigFiles.entries().forEach(
+ e ->
+ {
+ try {
+ // value is the config file full path, key is the system name which we want to prefix with to differentiate same name file.
+ LOGGER.debug("adding sync system {}'s config files {} to container local as {}", e.getKey(), e.getValue(), e.getKey()+"-<file-name>");
+ addAppLocalFiles(e.getValue(), e.getKey()+"-", appFilesDestDir,
+ Optional.<Map<String, LocalResource>>absent(), localFs);
+ } catch (IOException ioException) {
+ ioException.printStackTrace();
Review comment:
Shall we just log the IOE itself but print the longer trace?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -731,6 +752,25 @@ private void addContainerLocalResources(ApplicationId applicationId) throws IOEx
addAppLocalFiles(this.config.getString(GobblinYarnConfigurationKeys.CONTAINER_FILES_LOCAL_KEY),
Optional.<Map<String, LocalResource>>absent(), appFilesDestDir, localFs);
}
+
+ // handles all system configs files automatically, so it does not has to be mentioned explicitly as values of CONTAINER_FILES_LOCAL_KEY
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_COPY_SYSTEM_CONFIGS) && this.config.getBoolean(GobblinYarnConfigurationKeys.CONTAINER_COPY_SYSTEM_CONFIGS)) {
+ Path appFilesDestDir = new Path(containerWorkDir, GobblinYarnConfigurationKeys.APP_FILES_DIR_NAME);
+
+ Multimap<String, String> allSysConfigFiles = ConfigUtils.getAllSystemConfigFiles(this.config);
+ LOGGER.debug("adding all sync system's config files to container local.");
+ allSysConfigFiles.entries().forEach( e ->
+ {
+ try {
+ // value is the config file full path, key is the system name which we want to prefix with to differentiate same name file.
+ addAppLocalFiles(e.getValue(), e.getKey()+"-", appFilesDestDir,
+ Optional.<Map<String, LocalResource>>absent(), localFs);
+ } catch (IOException ioException) {
+ ioException.printStackTrace();
Review comment:
Same logging ask as before
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+ Map<String, ConfigObject> secureSystems = new HashMap<>();
+
+ if (allSysConfig == null) return secureSystems;
+
+ for (String systemName : allSysConfig.keySet()) {
+ ConfigObject sysConfig = allSysConfig.atPath(systemName).root();
+ sysConfig.get(IS_SECURE_KEY);
+ if (ConfigUtils.getBoolean(sysConfig.toConfig(), IS_SECURE_KEY, false)) {
+ secureSystems.put(systemName, sysConfig);
+ }
+ }
+ return secureSystems;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return multiple map of system name and its all config files regardless of the component.
+ */
+ public static Multimap<String, String> getAllSystemConfigFiles(Config config) {
+
+ Multimap<String, String> allSysConfFiles = LinkedHashMultimap.create();
+
+ getAllSystemConfig(config).entrySet();
+ ConfigObject allSysConfig = getAllSystemConfig(config);
+
+ if (allSysConfig == null) return allSysConfFiles;
+
+ for (Map.Entry<String, ConfigValue> sysConfig : allSysConfig.entrySet()) {
+
+ if (sysConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject allComponentConfigs = (ConfigObject) sysConfig.getValue();
+ for (Map.Entry<String, ConfigValue> componentConfig : allComponentConfigs.entrySet()) {
+ if (componentConfig.getValue().valueType().equals(ConfigValueType.OBJECT)) {
+ ConfigObject componentConfigValue = (ConfigObject) componentConfig.getValue();
+ Config configValue = componentConfigValue.toConfig();
+ if (configValue.hasPath(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY)) {
+ configValue.getList(GOBBLIN_SYNC_SYSTEM_CONFIG_FILES_KEY).forEach(item -> allSysConfFiles.put(sysConfig.getKey(), item.unwrapped().toString()));
+ }
+ }
+ }
+ }
+ }
+ return allSysConfFiles;
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of all system -> config for which the token management {@link ConfigUtils#IS_TOKEN_MGMT_ENABLED_KEY} is enabled.
+ */
+ public static Map<String, Config> getAllSecureHiveSystems(Config config){
+ Map<String, Config> secureSystems = new HashMap<>();
+
+ for (String systemName : config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY).keySet()){
+ Config sysConfig = config.getConfig(GOBBLIN_SYNC_SYSTEMS_KEY).getConfig(systemName);
+ if(ConfigUtils.getBoolean(sysConfig, IS_SECURE_KEY, false) && ConfigUtils.getBoolean(sysConfig, IS_TOKEN_MGMT_ENABLED_KEY, false) && sysConfig.hasPath(HIVE_SYSTEM_KEY)){
Review comment:
Not sure it is properly to leak contents of config in a utility class
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -731,6 +752,25 @@ private void addContainerLocalResources(ApplicationId applicationId) throws IOEx
addAppLocalFiles(this.config.getString(GobblinYarnConfigurationKeys.CONTAINER_FILES_LOCAL_KEY),
Optional.<Map<String, LocalResource>>absent(), appFilesDestDir, localFs);
}
+
+ // handles all system configs files automatically, so it does not has to be mentioned explicitly as values of CONTAINER_FILES_LOCAL_KEY
+ if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_COPY_SYSTEM_CONFIGS) && this.config.getBoolean(GobblinYarnConfigurationKeys.CONTAINER_COPY_SYSTEM_CONFIGS)) {
+ Path appFilesDestDir = new Path(containerWorkDir, GobblinYarnConfigurationKeys.APP_FILES_DIR_NAME);
Review comment:
It seems it needs to be named differently since it is under `containerWorkDir`
--
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] [gobblin] jhsenjaliya commented on a change in pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
Posted by GitBox <gi...@apache.org>.
jhsenjaliya commented on a change in pull request #3236:
URL: https://github.com/apache/gobblin/pull/3236#discussion_r646205346
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -569,4 +608,124 @@ public static Config resolveEncrypted(Config config, Optional<String> encConfigP
}
return ConfigFactory.parseMap(tmpMap).withFallback(config);
}
+
+ /**
+ *
+ * @param config
+ * @return ConfigObject ( config in tree ) of the {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ */
+ public static ConfigObject getAllSystemConfig(Config config) {
+ return (config.hasPath(GOBBLIN_SYNC_SYSTEMS_KEY) ? config.getObject(GOBBLIN_SYNC_SYSTEMS_KEY) : getInstance().root() );
+ }
+
+ /**
+ *
+ * @param config at path: {@link ConfigUtils#GOBBLIN_SYNC_SYSTEMS_KEY}
+ * @return map of system name to that system's config tree for all the systems that has security enabled
+ */
+ public static Map<String, ConfigObject> getAllSecureSystems(Config config) {
Review comment:
I am taking `configUtils` as a `GobblinConfigUtils` which should have config utility functions with gobblin's context. but sure it can be separated out.
--
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