hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Phabricator (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HIVE-4569) GetQueryPlan api in Hive Server2
Date Fri, 28 Jun 2013 23:36:21 GMT

    [ https://issues.apache.org/jira/browse/HIVE-4569?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13695920#comment-13695920
] 

Phabricator commented on HIVE-4569:
-----------------------------------

cwsteinbach has commented on the revision "HIVE-4569 [jira] GetQueryPlan api in Hive Server2".

INLINE COMMENTS
  ql/src/java/org/apache/hadoop/hive/ql/TaskStatus.java:1 Missing ASF license header.
  ql/src/java/org/apache/hadoop/hive/ql/exec/ExecDriver.java:1019 This looks like a debug
statement. Should it be removed?
  ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java:95 Can you add some comments here explaining
what each one of these states actually means? Also, do we need an UNKNOWN state? I included
one in the Thrift IDL OperationState, but in retrospect that was probably a mistake.
  service/if/TCLIService.thrift:34 As discussed earlier we shouldn't add this dependency to
the HS2 API. Please remove it and return the Task information in JSON or XML.
  service/if/TCLIService.thrift:41 We need to bump the version number since this patch extends
the HS2 API with new functionality. Can you also please add a comment here briefly summarize
what was added in the new version?
  service/if/TCLIService.thrift:594 Thrift allows you specify default values for optional
fields. I think we should set this value to 'false' by default.
  service/if/TCLIService.thrift:866 Just want to double-check that TTaskState and TTaskStatus
will be removed since the plan state will be serialized as JSON or XML, right?
  service/if/TCLIService.thrift:1003 Where is TGetQueryPlanReq? The comments at the top stipulate
that every RPC has it's own req/resp message pair.
  service/if/TCLIService.thrift:1006 Just double-checking that this will be changed to a string.
  service/if/TCLIService.thrift:1043 Please don't overload TExecuteStatementReq.
  service/src/java/org/apache/hive/service/cli/CLIService.java:149 Thrift makes it easy to
add additional optional parameters without breaking backward compatibility, but not Java.
I'd recommend creating a new executeStatementAsync call to ICLIService (and here) instead
of modifying the method signature. Also, that probably indicates that we should add a new
complimentary RPC to the HS2 Thrift IDL instead of using adding an optional parameter to ExecuteStatement
just to keep these things in sync.
  service/src/java/org/apache/hive/service/cli/CLIService.java:318 s/:getQueryPlan/: getQueryPlan/
  ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java:367 I don't think this method is thread-safe.
I recommend replacing the four boolean state variables (started, initialized, isdone, queued,
wth??) with the single TaskState enum you added and make sure that all access to this state
variable is synchronized.

REVISION DETAIL
  https://reviews.facebook.net/D11469

To: JIRA, jaideepdhok
Cc: cwsteinbach

                
> GetQueryPlan api in Hive Server2
> --------------------------------
>
>                 Key: HIVE-4569
>                 URL: https://issues.apache.org/jira/browse/HIVE-4569
>             Project: Hive
>          Issue Type: Bug
>          Components: HiveServer2
>            Reporter: Amareshwari Sriramadasu
>            Assignee: Jaideep Dhok
>         Attachments: git-4569.patch, HIVE-4569.D10887.1.patch, HIVE-4569.D11469.1.patch
>
>
> It would nice to have GetQueryPlan as thrift api. I do not see GetQueryPlan api available
in HiveServer2, though the wiki https://cwiki.apache.org/confluence/display/Hive/HiveServer2+Thrift+API
contains, not sure why it was not added.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message