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/03/20 01:02:07 UTC
[GitHub] [gobblin] autumnust commented on a change in pull request #3236: [GOBBLIN-1398] load system specific hive config to support multiple h…
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