Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

TAJO-1114: Improve ConfVars (SessionVar) to take a validator interface to check its input.#206

Closed
ykrips wants to merge 18 commits intoapache:masterfrom
ykrips:TAJO-1114
Closed

TAJO-1114: Improve ConfVars (SessionVar) to take a validator interface to check its input.#206
ykrips wants to merge 18 commits intoapache:masterfrom
ykrips:TAJO-1114

Conversation

@ykrips
Copy link
Copy Markdown

@ykrips ykrips commented Oct 20, 2014

No description provided.

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Oct 24, 2014

Even through it needs rebase, it was easy to merge the patch against the latest revision.

I'm reviewing this patch. I'll leave comments soon.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shell variable (i.e., ```${user.name}) is translated in Hadoop's Configuration. Actually, this config is just a string.

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Oct 25, 2014

Hi @ykrips,

I greatly appreciate your contribution. The patch looks really great. I love your design.

Actually, I didn't mention how we validate each config key. Of course, it is out of scope of this issue. Now, I just share it with you.

In my plan, Tajo will validate configs in two ways.

The first way is that TajoMaster validates each config when it starts up. If TajoMaster finds invalid config which probably will cause query runtime errors, TajoMaster will shutdown immediately and prints out detailed error messages. If so, we can earlier prohibit some kind of query runtime errors.

The second one is that TajoMaster checks each session setting action by client. If a client sets an invalid session variable, it will show some errors and prohibit putting the invalid variable into session variables.

If you fix the validator for tajo.username and rebase it, I'll finish the review.

Best regards,
Hyunsik

@ykrips
Copy link
Copy Markdown
Author

ykrips commented Oct 27, 2014

Hello @hyunsik ,
I will fix some missing points for your comments, and will rebase it.
On my implementation, I have overrided some functions of Configuration class to raise exceptions when invalid values are inserted. However, with your explanation, I feel that TajoMaster will throw exceptions, not TajoConf. I just wonder if I correctly understand your strategy.

@ykrips
Copy link
Copy Markdown
Author

ykrips commented Oct 27, 2014

Hello @hyunsik ,
I am apologized for my mistake on this pull request. I trying to rebase my local branch to github repository. But it corrupt ref files or any other files on my git repository. I'm sorry again for my mistake. And I will close and re-open this pull request.

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Oct 27, 2014

@ykrips

No problem :)
I'll leave the comment about your question in #214.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants