You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@crail.apache.org by raduioanstoica <gi...@git.apache.org> on 2018/02/27 14:18:04 UTC
[GitHub] incubator-crail pull request #7: Enable environment variable expansion in cr...
GitHub user raduioanstoica opened a pull request:
https://github.com/apache/incubator-crail/pull/7
Enable environment variable expansion in crail-site.conf
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/raduioanstoica/incubator-crail master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-crail/pull/7.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #7
----
commit 86a92a35867dae17bcf6d7c82dbd62e81e204ea3
Author: Radu Stoica <ra...@...>
Date: 2018-02-27T14:09:45Z
Enable environment variable expansion in crail-site.conf
----
---
[GitHub] incubator-crail issue #7: Enable environment variable expansion in crail-sit...
Posted by raduioanstoica <gi...@git.apache.org>.
Github user raduioanstoica commented on the issue:
https://github.com/apache/incubator-crail/pull/7
Sometimes it is easier to set dynamic configuration options through environment variables. This allows reusing the same Crail configuration file for similar deployments where only non-Crail specific parameters are different (such as the user name, HADOOP/SPARK/CRAIL deployment directories, hostnames).
---
[GitHub] incubator-crail pull request #7: Enable environment variable expansion in cr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-crail/pull/7
---
[GitHub] incubator-crail issue #7: Enable environment variable expansion in crail-sit...
Posted by raduioanstoica <gi...@git.apache.org>.
Github user raduioanstoica commented on the issue:
https://github.com/apache/incubator-crail/pull/7
At 3) I mean to extract the options directly from the code (from the CrailConstants-style classes) and update the crail-site.conf.template file in /conf. The options would be ideally commented out and include the default values and comments (if any). It would be much easier for a user to see all configuration options in one place.
There are many ways to do this. I guess we could start by having a look at Hadoop / Spark to see if they do something similar.
---
[GitHub] incubator-crail issue #7: Enable environment variable expansion in crail-sit...
Posted by animeshtrivedi <gi...@git.apache.org>.
Github user animeshtrivedi commented on the issue:
https://github.com/apache/incubator-crail/pull/7
Looks OK to me too.
@raduioanstoica yes, all of those issue need improvement in Crail code, specially unused or obsolete parameter names. I think we can have a look at other project and see how they do it.
I am not sure what you mean by (3) update automatically [...]?
---
[GitHub] incubator-crail pull request #7: Enable environment variable expansion in cr...
Posted by asqasq <gi...@git.apache.org>.
Github user asqasq commented on a diff in the pull request:
https://github.com/apache/incubator-crail/pull/7#discussion_r170948451
--- Diff: client/src/main/java/org/apache/crail/conf/CrailConfiguration.java ---
@@ -19,36 +19,80 @@
package org.apache.crail.conf;
+import org.apache.crail.utils.CrailUtils;
+import org.slf4j.Logger;
+
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.util.Properties;
import java.util.concurrent.ConcurrentHashMap;
-
-import org.apache.crail.utils.CrailUtils;
-import org.slf4j.Logger;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
public class CrailConfiguration {
private static final Logger LOG = CrailUtils.getLogger();
private ConcurrentHashMap<String, String> conf;
-
-
- public CrailConfiguration() throws IOException{
+
+ public CrailConfiguration() throws IOException {
conf = new ConcurrentHashMap<>();
Properties properties = loadProperties("crail-site.conf");
mergeProperties(properties);
}
+ private static String expandEnvVars(String input) {
+ if (null == input) {
+ return null;
+ }
+ // match ${ENV_VAR_NAME} or $ENV_VAR_NAME
+ Pattern p = Pattern.compile("\\$\\{(\\w+)\\}|\\$(\\w+)");
+ Matcher m = p.matcher(input);
+ StringBuffer output = new StringBuffer();
+ while (m.find()) {
+ String envVar;
+ if (m.group(1) != null)
+ envVar = m.group(1);
--- End diff --
Can you add braces to the if-else like every where else in the file?
---
[GitHub] incubator-crail issue #7: Enable environment variable expansion in crail-sit...
Posted by asqasq <gi...@git.apache.org>.
Github user asqasq commented on the issue:
https://github.com/apache/incubator-crail/pull/7
Looks good to me
---
[GitHub] incubator-crail issue #7: Enable environment variable expansion in crail-sit...
Posted by raduioanstoica <gi...@git.apache.org>.
Github user raduioanstoica commented on the issue:
https://github.com/apache/incubator-crail/pull/7
@animeshtrivedi
- If an environment variable is not set, a warning is printed and the option is set to an empty string (""). An exception will probably follow in case the option is not optional. Throwing an exception right away is probably better. We should do the same everywhere (e.g. when using CRAIL_HOME).
- Related to how configuration options are handled, several things could be improved: 1) specify for each parameter whether it is optional or not & data type + automatic validation; 2) print warnings for options that were set but are not used (to catch typos, obsolete old variable names); 3) update automatically crail-site.conf.template so users do not have to search in the code. Not sure though which is the best way to do this.
- I'll try to open a JIRA thread.
@asqasq
Sure, good point.
---
[GitHub] incubator-crail issue #7: Enable environment variable expansion in crail-sit...
Posted by PepperJo <gi...@git.apache.org>.
Github user PepperJo commented on the issue:
https://github.com/apache/incubator-crail/pull/7
Looks good to me. See discussion https://issues.apache.org/jira/browse/CRAIL-11
---
[GitHub] incubator-crail issue #7: Enable environment variable expansion in crail-sit...
Posted by animeshtrivedi <gi...@git.apache.org>.
Github user animeshtrivedi commented on the issue:
https://github.com/apache/incubator-crail/pull/7
Hey Radu, can you please open a JIRA (http://crail.incubator.apache.org/community/) for this pull request for discussion. We just do code review on github.
Also, what happens if a environment variable is not set? Is it possible to do some sort of sanity checks and throw an exception? Because if the variable is not set then probably it is bug (?)
---