You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by ykrips <gi...@git.apache.org> on 2014/10/20 15:04:43 UTC
[GitHub] tajo pull request: TAJO-1114: Improve ConfVars (SessionVar) to tak...
GitHub user ykrips opened a pull request:
https://github.com/apache/tajo/pull/206
TAJO-1114: Improve ConfVars (SessionVar) to take a validator interface to check its input.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ykrips/tajo TAJO-1114
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/206.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 #206
----
commit 412e2e6b8fcd208ded929df100d2ee80ee7a0281
Author: Jihun Kang <yk...@gmail.com>
Date: 2014-10-17T09:53:13Z
TAJO-1114: Improve ConfVars (SessionVar) to take a validator interface to check its input.
commit 44b991b43318da6d91758c3ef43232bd71414352
Author: Jihun Kang <yk...@gmail.com>
Date: 2014-10-20T06:47:34Z
Merge remote-tracking branch 'upstream/master' into TAJO-1114
commit 76720f853dd22b245207d2de8e49fcc9554562f5
Author: Jihun Kang <yk...@gmail.com>
Date: 2014-10-20T07:13:01Z
TAJO-1114: Improve ConfVars (SessionVar) to take a validator interface to check its input.
commit f99e7edcfa51c2955da7a54af4a2edf7b11c6068
Author: Jihun Kang <yk...@gmail.com>
Date: 2014-10-20T07:13:53Z
TAJO-1114: Improve ConfVars (SessionVar) to take a validator interface to check its input.
commit 0ad448d1e51a35778b527a952dbd2c36345579e5
Author: Jihun Kang <yk...@gmail.com>
Date: 2014-10-20T07:15:00Z
Merge remote-tracking branch 'upstream/master' into TAJO-1114
commit ce0fb6d44e5782c28618b735d95b10c57d3f4692
Author: Jihun Kang <yk...@gmail.com>
Date: 2014-10-20T12:47:27Z
TAJO-1114: Improve ConfVars (SessionVar) to take a validator interface to check its input.
commit 3f3a23a138cc523316cd2b28f1251921a55d404e
Author: Jihun Kang <yk...@gmail.com>
Date: 2014-10-20T13:02:30Z
TAJO-1114: Improve ConfVars (SessionVar) to take a validator interface to check its input.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1114: Improve ConfVars (SessionVar) to tak...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/206#issuecomment-60566349
@ykrips
No problem :)
I'll leave the comment about your question in #214.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1114: Improve ConfVars (SessionVar) to tak...
Posted by ykrips <gi...@git.apache.org>.
Github user ykrips closed the pull request at:
https://github.com/apache/tajo/pull/206
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1114: Improve ConfVars (SessionVar) to tak...
Posted by ykrips <gi...@git.apache.org>.
Github user ykrips commented on the pull request:
https://github.com/apache/tajo/pull/206#issuecomment-60565026
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1114: Improve ConfVars (SessionVar) to tak...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/206#issuecomment-60473494
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1114: Improve ConfVars (SessionVar) to tak...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/206#issuecomment-60461282
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1114: Improve ConfVars (SessionVar) to tak...
Posted by ykrips <gi...@git.apache.org>.
Github user ykrips commented on the pull request:
https://github.com/apache/tajo/pull/206#issuecomment-60557163
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1114: Improve ConfVars (SessionVar) to tak...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/206#discussion_r19374125
--- Diff: tajo-common/src/main/java/org/apache/tajo/conf/TajoConf.java ---
@@ -143,54 +156,61 @@ public static int setDateOrder(int dateOrder) {
///////////////////////////////////////////////////////////////////////////////////////
// a username for a running Tajo cluster
- ROOT_DIR("tajo.rootdir", "file:///tmp/tajo-${user.name}/"),
- USERNAME("tajo.username", "${user.name}"),
+ ROOT_DIR("tajo.rootdir", "file:///tmp/tajo-${user.name}/",
+ Validators.groups(Validators.notNull(), Validators.pathUrl())),
+ USERNAME("tajo.username", "${user.name}",
+ Validators.groups(Validators.notNull(), Validators.shellVar())),
--- End diff --
Shell variable (i.e., ```${user.name}) is translated in Hadoop's Configuration. Actually, this config is just a string.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---