GitHub user jinossy opened a pull request:

https://github.com/apache/tajo/pull/99

TAJO-985: Client API should be async

The query status synchronization on state machine always wait for time (event processing time) before status changes. If a lot of time are being processed to the event dispatcher, client api(getStatus, getProgress) will be waiting for changed event status. and query will be session timed out.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jinossy/tajo TAJO-985

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/tajo/pull/99.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 #99

commit 565744dea7ebcd80de100741465cab606fdad068
Author: jinossy <jinossy@gmail.com>
Date: 2014-07-29T08:54:00Z

TAJO-985: Client API should be async

commit e4eb0fe704a7499858ba4821910e75f1577c79a8
Author: jinossy <jinossy@gmail.com>
Date: 2014-07-30T02:50:58Z

fixed wrong progress

commit e6326d8b6dd26742de026dba1d0a4d34f2bc99cd
Author: jinossy <jinossy@gmail.com>
Date: 2014-07-30T02:57:20Z

applied to web ui

commit 59425a86fae13afb90cda6c91fbc2ce02bfdc8ed
Author: jinossy <jinossy@gmail.com>
Date: 2014-07-31T07:01:43Z

added event executor

Github user hyunsik commented on a diff in the pull request:

https://github.com/apache/tajo/pull/99#discussion_r15741540

— Diff: tajo-client/src/main/java/org/apache/tajo/cli/TajoCli.java —
@@ -487,7 +487,7 @@ private void waitForQueryCompleted(QueryId queryId) {
while (true) {
// TODO - configurable
status = client.getQueryStatus(queryId);

• if(status.getState() == QueryState.QUERY_MASTER_INIT || status.getState() == QueryState.QUERY_MASTER_LAUNCHED) {
  • • End diff –

The line is longer than 120 columns.

Github user hyunsik commented on a diff in the pull request:

https://github.com/apache/tajo/pull/99#discussion_r15741545

— Diff: tajo-client/src/main/java/org/apache/tajo/cli/TajoCli.java —
@@ -487,7 +487,7 @@ private void waitForQueryCompleted(QueryId queryId) {
while (true) {
// TODO - configurable
status = client.getQueryStatus(queryId);

• if(status.getState() == QueryState.QUERY_MASTER_INIT || status.getState() == QueryState.QUERY_MASTER_LAUNCHED) {
  + if(status.getState() == QueryState.QUERY_NEW || status.getState() == QueryState.QUERY_MASTER_INIT || status.getState() == QueryState.QUERY_MASTER_LAUNCHED) {
  • • End diff –

The line is longer than 120 columns.

Github user jinossy commented on the pull request:

https://github.com/apache/tajo/pull/99#issuecomment-51149887

Thank you for the review. I reflected your comments.

Github user hyunsik commented on a diff in the pull request:

https://github.com/apache/tajo/pull/99#discussion_r16035452

— Diff: tajo-core/src/main/java/org/apache/tajo/master/querymaster/Query.java —
@@ -337,15 +340,24 @@ public SubQuery getSubQuery(ExecutionBlockId id)

• public QueryState getState() {
• readLock.lock();
• try { - return stateMachine.getCurrentState(); - }

  finally {

• readLock.unlock();
  + protected QueryState getState(boolean async) {
  • • End diff –

I think that the variable name ```async``` should be nonblocking.

Github user hyunsik commented on the pull request:

https://github.com/apache/tajo/pull/99#issuecomment-51736398

The main idea looks good to me. It will mitigate lock contention problem.

I have one suggestion. How about changing getState(boolean) and getState signatures into different names? Since the uses of them may be sensitive, we need to know what we invoke more apparently.

I would like to suggest getSynchronizedState() and getState(). It's just a suggestion. The decision is up to you.

Github user jinossy commented on the pull request:

https://github.com/apache/tajo/pull/99#issuecomment-51737829

@hyunsik good idea.

Github user jinossy commented on the pull request:

https://github.com/apache/tajo/pull/99#issuecomment-51748431

I’ve reflects your suggestion.

Github user hyunsik commented on the pull request:

https://github.com/apache/tajo/pull/99#issuecomment-52447449

+1

The patch looks nice to me. Ship it.

Github user hyunsik commented on the pull request:

https://github.com/apache/tajo/pull/99#issuecomment-52447530

The word 'async' was changed during the review. So, I think that the issue title also should be changed to proper name. Could you change the title before committing?

Github user jinossy commented on the pull request:

https://github.com/apache/tajo/pull/99#issuecomment-52450810

Thank you for the review.
I've just committed it.

Github user jinossy closed the pull request at:

https://github.com/apache/tajo/pull/99

SUCCESS: Integrated in Tajo-master-build #337 (See https://builds.apache.org/job/Tajo-master-build/337/)
TAJO-985: Client API should be non-blocking. (jinho) (jhkim: rev 60858b25c46fceedf70b8583f1d7ef1383a19c6b)

• tajo-core/src/main/java/org/apache/tajo/master/querymaster/QueryMasterTask.java
• CHANGES
• tajo-core/src/main/java/org/apache/tajo/worker/TajoWorkerClientService.java
• tajo-core/src/main/java/org/apache/tajo/master/querymaster/QueryMaster.java
• tajo-core/src/test/java/org/apache/tajo/client/TestTajoClient.java
• tajo-core/src/main/java/org/apache/tajo/master/querymaster/SubQuery.java
• tajo-core/src/main/java/org/apache/tajo/master/querymaster/Query.java
• tajo-client/src/main/java/org/apache/tajo/client/TajoClient.java
• tajo-core/src/main/java/org/apache/tajo/worker/TajoResourceAllocator.java
• tajo-client/src/main/java/org/apache/tajo/cli/TajoCli.java
• tajo-client/src/main/java/org/apache/tajo/client/TajoAdmin.java