[
https://issues.apache.org/jira/browse/HIVE-14374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15399734#comment-15399734
]
Sahil Takiar commented on HIVE-14374:
-------------------------------------
A lot of the reflection code is also in {{org.apache.hive.beeline.Reflector}}.
I agree this code requires some cleanup. I believe [~vihangk1] has brought up
the issue that it makes the code a lot less readable.
[~pvary] are there a lot of scenarios where a configuration parameter should
only be set via the command, but not in the configuration file (or vice versa)?
If not I would say it may be simpler to just have one annotation
{{ConfigurationKey}} that indicates this setter method should be use as a
configuration key. The advantage here is that users don't need to track which
configuration keys can be configured from which location (e.g. command line or
properties file).
If we are going to invest time in cleaning up this code, it may be worth
considering whether we should just use a standard Java Command Line Options
Parser (commons-cli, jopt, jcommander, etc.). A simple pattern could be to just
parse in the BeeLine configuration file, and then parse in the command line
options. If a configuration key is set in both places, the command line option
takes precedence.
> BeeLine argument, and configuration handling cleanup
> ----------------------------------------------------
>
> Key: HIVE-14374
> URL: https://issues.apache.org/jira/browse/HIVE-14374
> Project: Hive
> Issue Type: Improvement
> Components: Beeline
> Affects Versions: 2.2.0
> Reporter: Peter Vary
> Assignee: Peter Vary
>
> BeeLine uses reflection, to set the BeeLineOpts attributes when parsing
> command line arguments, and when loading the configuration file.
> This means, that creating a setXXX, getXXX method in BeeLineOpts is a
> potential risk of exposing an attribute for the user unintentionally. There
> is a possibility to exclude an attribute from saving the value in the
> configuration file with the Ignore annotation. This does not restrict the
> loading or command line setting of these parameters which means there are
> many undocumented "features" as-is, like setting the lastConnectedUrl,
> allowMultilineCommand, maxHeight, trimScripts, etc. from command line.
> This part of the code needs a little cleanup.
> I think we should make this exposure more explicit, and be able to
> differentiate the configurable options depending on the source (command line,
> and configuration file), so I propose to create a mechanism to tell
> explicitly which BeeLineOpts attributes are settable by command line, and
> configuration file, and every other attribute should be inaccessible by the
> user of the beeline cli.
> One possible solution could be two annotations like these:
> - CommandLineOption - there could be a mandatory text parameter here, so the
> developer had to provide the help text for it which could be displayed to the
> user
> - ConfigurationFileOption - no text is required here
> Something like this:
> - This attribute could be provided by command line, and from a configuration
> file too:
> {noformat}
> @CommandLineOption("automatically save preferences")
> @ConfigurationFileOption
> public void setAutosave(boolean autosave) {
> this.autosave = autosave;
> }
> public void getAutosave() {
> return this.autosave;
> }
> {noformat}
> - This attribute could be set through the configuration only
> {noformat}
> @ConfigurationFileOption
> public void setLastConnectedUrl(String lastConnectedUrl) {
> this.lastConnectedUrl = lastConnectedUrl;
> }
>
> public String getLastConnectedUrl()
> {
> return lastConnectedUrl;
>
}
>
{noformat}
> - Attribute could be set through command line only - I think this is not too
> relevant, but possible
> {noformat}
> @CommandLineOption("specific command line option")
> public void setSpecificCommandLineOption(String specificCommandLineOption) {
>
this.specificCommandLineOption = specificCommandLineOption;
>
}
>
> public String getSpecificCommandLineOption() {
>
return specificCommandLineOption;
>
}
>
{noformat}
> - Attribute could not be set
> {noformat}
> public static Env getEnv() {
>
return env;
>
}
>
public static void setEnv(Env envToUse) {
>
env = envToUse;
>
}
> {noformat}
> Accouring to our previous conversations, I think you might be interested in:
> [~spena], [~vihangk1], [~aihuaxu], [~ngangam], [~ychena], [~xuefuz]
> but anyone is welcome to discuss this.
> What do you think about the proposed solution?
> Any better ideas, or extensions?
> Thanks,
> Peter
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
