Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201704051819.v35IJ4O6009045@ip-10-146-233-104.ec2.internal>
Date: Wed, 5 Apr 2017 18:19:04 +0000
From: "Marcel Kornacker (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: marcel@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-2550=3A_Switch_to_per-query_exec_rpc=0A?=
In-Reply-To: <gerrit.1491237713000.I20769e420711737b6b385c744cef4851cee3facd@gerrit.cloudera.org>
References: <gerrit.1491237713000.I20769e420711737b6b385c744cef4851cee3facd@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Wed, 05 Apr 2017 18:19:10 -0000

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-2550: Switch to per-query exec rpc
......................................................................


Patch Set 3:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/6535/3/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS3, Line 381: the total number
             : // of fragment instances
> what's that?
Done


PS3, Line 386: i32
> Types.TFragmentIdx ?
Done


PS3, Line 412: 6: list<TPlanFragmentDestination> destinations
> isn't that common across all instances?
Done


PS3, Line 414: fragment
> instance?
Done


PS3, Line 435: coord_state_idx
> not clear why the backend will care what index it was represented by in a c
Done


Line 441:   4: list<TPlanFragmentCtx> fragment_ctxs
> are these in any particular order?
i explained the ordering for fragment_instance_ctxs


Line 453: // ReportExecStatus
> other RPC names have a blank line after them, which makes them easier to fi
Done


PS3, Line 552: overall
> what does "overall" mean here?  this makes it seem like it's the merged sta
changed code to match description


PS3, Line 560: insert_exec_status
> I guess this will hold the insert status for all instances, right?
rephrased


Line 563:   7: optional map<ErrorCodes.TErrorCode, TErrorLogEntry> error_log;
> and same here. this will be the aggregated error log across all instances?
rephrased


PS3, Line 572: CancelPlanFragment
> CancelQueryFInstances
Done


Line 669: 
> It would be helpful to add the RPC comment to show which RPC the following 
Done


Line 699: 
> // PublishFilter
Done


PS3, Line 734: ReportExecStatus
> Eventually this will report status for all the instances (at least in the p
well, it's overall backend execution status (this could report an error even before any fragment instances are started), so that's why i thought 'exec status' is still the right scope


Line 739:   TCancelQueryFInstancesResult CancelQueryFInstances(
i didn't want to name this CancelQuery, because that sounds too much like a client-related rpc


-- 
To view, visit http://gerrit.cloudera.org:8080/6535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes