Skip to content
This repository has been archived by the owner on Jul 7, 2022. It is now read-only.

Enable environment variable expansion in crail-site.conf #7

Closed
wants to merge 2 commits into from

Conversation

raduioanstoica
Copy link
Contributor

No description provided.

@raduioanstoica
Copy link
Contributor Author

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).

@animeshtrivedi
Copy link
Contributor

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 (?)

Copy link
Member

@asqasq asqasq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment, otherwise looks fine with me.

while (m.find()) {
String envVar;
if (m.group(1) != null)
envVar = m.group(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add braces to the if-else like every where else in the file?

@raduioanstoica
Copy link
Contributor Author

@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.

@animeshtrivedi
Copy link
Contributor

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 [...]?

@raduioanstoica
Copy link
Contributor Author

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.

Copy link
Member

@asqasq asqasq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@PepperJo
Copy link
Contributor

Looks good to me. See discussion https://issues.apache.org/jira/browse/CRAIL-11

@asfgit asfgit closed this in 9b857f5 Mar 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants