> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/if/TCLIService.thrift, line 395 > > <https://reviews.apache.org/r/15151/diff/3/?file=381909#file381909line395> > > > > Please reuse TStatus instead of adding a new struct.
My understanding is that TStatus encapsulates the status of an RPC request (as pointed out by Prasad earlier). It has a required TStatusCode field as well which corresponds to the status of an RPC call. Would it not be better to expand TGetOperationStatusResp with sqlState, errorCode and errorMessage? > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java, > > line 1 > > <https://reviews.apache.org/r/15151/diff/3/?file=381906#file381906line1> > > > > Does this patch make any changes to CLIServiceTest or > > ThriftCLIServiceTest, or does it just move these files from service/ to > > itests/ ? If it does make changes can we move the files in a different > > patch? It moves these files, but also adds test for this feature + some minor refactoring. > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/if/TCLIService.thrift, line 50 > > <https://reviews.apache.org/r/15151/diff/3/?file=381909#file381909line50> > > > > Need to add HIVE_CLI_SERVICE_PROTOCOL_V5 and update any references to > > HIVE_CLI_SERVICE_PROTOCOL_V4. Done > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/src/java/org/apache/hive/service/cli/CLIService.java, line 274 > > <https://reviews.apache.org/r/15151/diff/3/?file=381910#file381910line274> > > > > Please push this logic into OperationManager.getOperationStatus() Done > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/if/TCLIService.thrift, line 917 > > <https://reviews.apache.org/r/15151/diff/3/?file=381909#file381909line917> > > > > This modification will break compatibility between > > upversion/downversion clients and servers since it modifies the type of an > > existing field. > > > > It's possible to avoid this problem by instead adding a new "TStatus > > operationStatus" field. Sorry about the slip, will expand TGetOperationStatusResp with a new optional field(s). Would appreciate your thoughts on my reply in the previous comment regarding TStatus. > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/src/java/org/apache/hive/service/cli/operation/OperationManager.java, > > line 147 > > <https://reviews.apache.org/r/15151/diff/3/?file=381918#file381918line147> > > > > s/getOperationRunException/getOperationException/ Done > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, > > line 71 > > <https://reviews.apache.org/r/15151/diff/3/?file=381919#file381919line71> > > > > s/runException/operationException/ Done > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line > > 73 > > <https://reviews.apache.org/r/15151/diff/3/?file=381917#file381917line73> > > > > Would it make sense to replace getState() and getRunException() with a > > getStatus() method that returns an object wrapping the operationState and > > operationException? > > > > If not, please change the name of getRunException to getException(), > > and add a comment explaining under what conditions this method will return > > a non-null value. Done > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, > > line 324 > > <https://reviews.apache.org/r/15151/diff/3/?file=381919#file381919line324> > > > > May as well just push this into Operation. Done > On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote: > > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java, > > line 307 > > <https://reviews.apache.org/r/15151/diff/3/?file=381921#file381921line307> > > > > Todo: set the status information. Could you elaborate this a bit more? Thanks for the feedback. - Vaibhav ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15151/#review28677 ----------------------------------------------------------- On Nov. 11, 2013, 7:23 p.m., Vaibhav Gumashta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15151/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2013, 7:23 p.m.) > > > Review request for hive, Prasad Mujumdar and Thejas Nair. > > > Bugs: HIVE-5230 > https://issues.apache.org/jira/browse/HIVE-5230 > > > Repository: hive-git > > > Description > ------- > > [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support > for async execution in HS2. When a background thread gets an error, currently > the client can only poll for the operation state and also the error with its > stacktrace is logged. However, it will be useful to provide a richer error > response like thrift API does with TStatus (which is constructed while > building a Thrift response object). > > > Diffs > ----- > > > itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java > PRE-CREATION > > itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java > PRE-CREATION > jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf > service/if/TCLIService.thrift 1f49445 > service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 > service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f > service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java > 9dca874 > service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 > service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 > service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 > service/src/java/org/apache/hive/service/cli/OperationStatus.java > PRE-CREATION > service/src/java/org/apache/hive/service/cli/operation/Operation.java > 6f4b8dc > > service/src/java/org/apache/hive/service/cli/operation/OperationManager.java > bcdb67f > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java > 4ee1b74 > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java > 9df110e > > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java > 9bb2a0f > service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a > > service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java > ff7166d > > Diff: https://reviews.apache.org/r/15151/diff/ > > > Testing > ------- > > > Thanks, > > Vaibhav Gumashta > >
