You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by dkhwangbo <gi...@git.apache.org> on 2016/01/05 04:18:42 UTC
[GitHub] tajo pull request: TAJO-2033: Printing out query status with progr...
GitHub user dkhwangbo opened a pull request:
https://github.com/apache/tajo/pull/924
TAJO-2033: Printing out query status with progress bar in TSQL
Currently, TSQL print out query running progress and query running time line-by-line. To improve this UI, I adopt simple progress bar.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dkhwangbo/tajo TAJO-2033
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/924.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 #924
----
commit e6681b5d575810e4860a2d3accbb28eec725fe07
Author: Dongkyu Hwangbo <hw...@gmail.com>
Date: 2015-12-17T09:03:16Z
Add ConsolePrinter
commit cdf6d706f45a8bf41613cf52a5b5db70f1ed0b25
Author: Dongkyu Hwangbo <hw...@gmail.com>
Date: 2015-12-17T09:04:58Z
Add several function using ConsolePrinter
commit c5cceb0c382a99da0eabd4eca239649c866d05e7
Author: Dongkyu Hwangbo <hw...@gmail.com>
Date: 2016-01-04T07:30:39Z
adopt progressbar
commit 1eeeb5f7085514b7f47c5fe83bdfd92d2ff98325
Author: Dongkyu Hwangbo <hw...@gmail.com>
Date: 2016-01-04T07:36:18Z
remove unused method
commit 2e93d575fb4d2c2f16279dc2b63f2e238ee6f449
Author: Dongkyu Hwangbo <hw...@gmail.com>
Date: 2016-01-05T02:07:01Z
remove unnecessarily written
commit 2ee4bac481def1d1d2710e5fba0cff720b0b5141
Author: Dongkyu Hwangbo <hw...@gmail.com>
Date: 2016-01-05T02:41:07Z
remove ConsolePrinter
commit afa534c9a5b47024285aed74310d314f656cf2a8
Author: Dongkyu Hwangbo <hw...@gmail.com>
Date: 2016-01-05T03:16:25Z
make code lighten
----
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/924#issuecomment-184564891
+1 LGTM.
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52960997
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/DefaultTajoCliOutputFormatter.java ---
@@ -206,4 +278,30 @@ public static String parseErrorMessage(String message) {
return message;
}
+
+ private static boolean detectRealTerminal()
--- End diff --
You seem to borrow this code from Presto. If so, you need to add a comment explicitly.
And why did you exclude comments?
---
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-2033: Printing out query status with progr...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52963036
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/TajoCli.java ---
@@ -619,6 +623,10 @@ private void waitForQueryCompleted(QueryId queryId) {
}
}
+ if (isProgressPrinting == true) {
+ displayFormatter.printProgress(sout, status);
+ sout.println();
--- End diff --
```printProgress()`` only express the progress of query. To print query result in next line, add a new line is needed.
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/924#issuecomment-173395287
Hi @dkhwangbo, your patch looks good to me. However, I'm concerned with the test failure at https://travis-ci.org/apache/tajo/builds/101996748. I've never seen before, so would you investigate what caused this failure?
---
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-2033: Printing out query status with progr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/924
---
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-2033: Printing out query status with progr...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on the pull request:
https://github.com/apache/tajo/pull/924#issuecomment-184490990
@jihoonson Hi. Thanks for your comment. I reflect your mention. Please review it again.
---
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-2033: Printing out query status with progr...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52962563
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/TajoCli.java ---
@@ -607,8 +608,11 @@ private void waitForQueryCompleted(QueryId queryId) {
continue;
}
- if (TajoClientUtil.isQueryRunning(status.getState()) || status.getState() == QueryState.QUERY_SUCCEEDED) {
+ if (TajoClientUtil.isQueryRunning(status.getState())) {
displayFormatter.printProgress(sout, status);
+ if (isProgressPrinting == false) {
--- End diff --
OK. I'll fix it.
---
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-2033: Printing out query status with progr...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on the pull request:
https://github.com/apache/tajo/pull/924#issuecomment-174133189
@jihoonson @hyunsik Hi! Thanks for your helpful comment. I make using ```println()``` instead of ```print()``` in ```SetCommand::set``` you mentioned. To make it, I rewrite some part in ```TajoCli::waitforQueryCompleted``` . Can I get some review message about updated commit?
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/924#issuecomment-184567277
I found some codes which don't follow our coding convention, and fixed them before I commit.
---
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-2033: Printing out query status with progr...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52962571
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/TajoCli.java ---
@@ -619,6 +623,10 @@ private void waitForQueryCompleted(QueryId queryId) {
}
}
+ if (isProgressPrinting == true) {
--- End diff --
OK. I'll fix it.
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52961066
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/TajoCli.java ---
@@ -619,6 +623,10 @@ private void waitForQueryCompleted(QueryId queryId) {
}
}
+ if (isProgressPrinting == true) {
--- End diff --
```[val] == [boolean]``` is a bad idiom.
---
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-2033: Printing out query status with progr...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52966518
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/DefaultTajoCliOutputFormatter.java ---
@@ -206,4 +278,33 @@ public static String parseErrorMessage(String message) {
return message;
}
+
+ /**
+ * borrowed from Presto
+ */
+ private static boolean detectRealTerminal()
--- End diff --
Oh, I see. When I borrowed this method, I wondered about fitness of this comments to tajo. Currently, This comments are written for Presto, not Tajo. So I just confused about this comments to include this PR. But, After Testing this PR in many individual environment, I take conviction about this method, but I missed to include its comments. This is my mistake. I'll add comments.
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52961100
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/TajoCli.java ---
@@ -619,6 +623,10 @@ private void waitForQueryCompleted(QueryId queryId) {
}
}
+ if (isProgressPrinting == true) {
+ displayFormatter.printProgress(sout, status);
+ sout.println();
--- End diff --
Here, you added a new line. What does this new line mean? Please add some comments on that.
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52961062
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/TajoCli.java ---
@@ -607,8 +608,11 @@ private void waitForQueryCompleted(QueryId queryId) {
continue;
}
- if (TajoClientUtil.isQueryRunning(status.getState()) || status.getState() == QueryState.QUERY_SUCCEEDED) {
+ if (TajoClientUtil.isQueryRunning(status.getState())) {
displayFormatter.printProgress(sout, status);
+ if (isProgressPrinting == false) {
--- End diff --
```[val] == [boolean]``` is a bad idiom.
---
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-2033: Printing out query status with progr...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52962556
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/DefaultTajoCliOutputFormatter.java ---
@@ -206,4 +278,30 @@ public static String parseErrorMessage(String message) {
return message;
}
+
+ private static boolean detectRealTerminal()
--- End diff --
Yes. You're right. I've not known the rule about code redistribution. I'll add the comment explicitly.
---
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-2033: Printing out query status with progr...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on the pull request:
https://github.com/apache/tajo/pull/924#issuecomment-173780673
@jihoonson Hi! Thanks for your reply. To investigate the test failure you mentioned, I ran that test code on my local machine. There's no failure. Also, I pushed several travis CI trigger and same failure was not appeared.
(On the other hand, Another failure was appeared in https://travis-ci.org/apache/tajo/builds/103794425. )
This PR has no modification about these failured test's code. Condition difference in build-by-build has no relation about this PR, I guess. It means, I find no evidence yet about this problem. Can I get some advice or tip to resolve it?
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52965368
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/DefaultTajoCliOutputFormatter.java ---
@@ -206,4 +278,33 @@ public static String parseErrorMessage(String message) {
return message;
}
+
+ /**
+ * borrowed from Presto
+ */
+ private static boolean detectRealTerminal()
--- End diff --
My previous question was too short, so you may miss my point.
I mean, why are the comments in Presto's detectRealTerminal() excluded when you borrow?
Please see https://github.com/facebook/presto/blob/master/presto-cli/src/main/java/com/facebook/presto/cli/ConsolePrinter.java. It will be helpful for us as well.
---
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-2033: Printing out query status with progr...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/924#discussion_r52965224
--- Diff: tajo-cli/src/main/java/org/apache/tajo/cli/tsql/DefaultTajoCliOutputFormatter.java ---
@@ -206,4 +278,30 @@ public static String parseErrorMessage(String message) {
return message;
}
+
+ private static boolean detectRealTerminal()
--- End diff --
This is not a rule, but is required for code maintenance.
---
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.
---