You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Andrew Sherman (Code Review)" <ge...@cloudera.org> on 2019/01/02 18:20:33 UTC
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Andrew Sherman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12142
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
IMPALA-7468: Port CancelQueryFInstances() to KRPC.
When the Coordinator needs to cancel a query (usually because a user has
hit Control-C), it does this by sending a CancelQueryFInstances message
to each fragment instance. This change switches this code to use KRPC.
Add new protobuf definitions for the messages, and remove the old thrift
definitions. Move the server-side implementation of Cancel() from
ImpalaInternalService to ControlService. Rework the scheduler so
that the FInstanceExecParams always contains the KRPC address of the
fragment executors, this address can then be used if a query is to be
cancelled.
For now keep the KRPC calls to CancelQueryFInstances() as synchronous.
While moving the client-side code, remove the fault injection code that
was inserted with FAULT_INJECTION_SEND_RPC_EXCEPTION and
FAULT_INJECTION_RECV_RPC_EXCEPTION (triggered by running impalad with
--fault_injection_rpc_exception_type=1) as this tickles code in
client-cache.h which is now not used.
TESTING:
Ran all end-to-end tests.
No new tests as test_cancellation.py provides good coverage.
Checked in debugger that DebugAction style fault injection (triggered
from test_cancellation.py) was working correctly.
Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
12 files changed, 165 insertions(+), 105 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12142/1
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1700/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Jan 2019 01:35:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1725/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Jan 2019 19:12:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1699/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Jan 2019 19:52:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Jan 2019 21:02:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Zoram Thanga (Code Review)" <ge...@cloudera.org>.
Zoram Thanga has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 3: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h@294
PS1, Line 294: Status DoCancelQueryFInstancesRrpcWithRetry(
> Up to now I have just been using what the clang-format rules dictate. In cl
Actually, I wasn't aware that we had these style rules defined in the clang-format files. I was only commenting on this aspect when viewing the changed code in relation to the existing code. I would just stick with what the existing code is following.
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h@104
PS1, Line 104: const TNetworkAddress& krpc_host, int per_fragment_instance_idx,
> There is no trailing whitespace I could find.
Ok. Thanks.
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@163
PS1, Line 163: optional UniqueIdPB query_id = 1;
> OK you made me think!
That makes perfect sense. Thanks for the link!
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@179
PS1, Line 179: // Cancellation is asynchronous (in the sense that this call may return before the
> When the coordinator cancels a query it sends CancelQueryFInstancesRequestP
Thanks for clarifying that.
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Jan 2019 19:45:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Zoram Thanga (Code Review)" <ge...@cloudera.org>.
Zoram Thanga has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h@294
PS1, Line 294: Status DoCancelQueryFInstancesRrpcWithRetry(
Nit: The prevailing style seems to place reference ('&') and pointer ('*') symbols next to the pointed-to-type, rather than next to the variable name. Comment applies for the entire review.
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h@104
PS1, Line 104: const TNetworkAddress& krpc_host, int per_fragment_instance_idx,
Nit: I wonder if there are trailing whitespaces in these lines? I don't understand why some lines are marked as changed...
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.h@433
PS1, Line 433: void ComputeFragmentExecParams(const BackendConfig executor_config,
Nit: Please retain existing placement of '&' and '*'.
Also: Are you intentionally passing executor_config by value? Here and in the following functions?
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.cc@333
PS1, Line 333: ComputeFragmentExecParams(executor_config, plan_exec_info,
Nit: Trailing spaces?
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@163
PS1, Line 163: optional UniqueIdPB query_id = 1;
Don't understand how the query id can be optional...
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@179
PS1, Line 179: // Cancellation is asynchronous.
Commit message says cancellation is synchronous. Do you want to change this comment until async cancellation is implemented?
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Jan 2019 19:29:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 3:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/12142/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12142/3//COMMIT_MSG@9
PS3, Line 9: (usually because a user has
: hit Control-C)
Not that important, but is this really true? Some other cancellation situations:
- coordinator decides to cancel the query because all of the results have already been returned (eg. because of an outer limit)
- cancellation is initiated from a different interface (eg. Hue, CM, the webui)
I also bring this up to ensure that you've considered and tested these cases (though of course I don't expect them to be any different)
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.h@239
PS3, Line 239: Execution backend.
Thrift execution backend?
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h@294
PS1, Line 294: Status DoCancelQueryFInstancesRrpcWithRetry(
> Actually, I wasn't aware that we had these style rules defined in the clang
I believe our current clang-format rules will tell you to put the symbols next to the type in all cases, which is what we want.
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.cc@427
PS3, Line 427: auto
We generally avoid the use of auto as it hurts readability.
Obviously, 'auto' still appears in a number of places in Impala, but generally where there is a very lengthy type where the details are uninteresting/obvious, such as an iterator.
As far as I can tell, we don't have any guidelines documented about this in our style guide. We should probably add some.
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.cc@440
PS3, Line 440: DoCancelQueryFInstancesRrpcWithRetry
Is there a reasonable way to make this generic like DoRpcWithRetry()? We'll need something like that in other places anyways, eg. when porting RemoteShutdown().
If not, I would probably just inline this instead of having a separate function.
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/scheduling/query-schedule.h@88
PS3, Line 88: execution backend
Thrift execution backend?
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.h@41
PS3, Line 41: public:
Not your code, but while you're here would you mind fixing the spacing in this file? (everything has one too many spaces, except GetProxy())
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.h@63
PS3, Line 63: /// Cancel any executing fragment instances for the query id specified in 'req'.
nit: could you move this under ReportExecStatus() to keep all of the rpc handling functions together (and the same in the .cc)
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.h@64
PS3, Line 64: void
virtual
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.cc@173
PS3, Line 173: "query"
Since this is a constant, its not really necessary to Substitute() it.
http://gerrit.cloudera.org:8080/#/c/12142/3/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:
http://gerrit.cloudera.org:8080/#/c/12142/3/common/protobuf/control_service.proto@181
PS3, Line 181: rpc CancelQueryFInstances(CancelQueryFInstancesRequestPB) returns (CancelQueryFInstancesResponsePB);
Long line.
Looks like our formatting infrastructure (eg. clang-format and the jenkins bot) don't currently support .proto files. We should probably fix that
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Jan 2019 20:36:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 3:
(11 comments)
Thanks for the code reviews
http://gerrit.cloudera.org:8080/#/c/12142/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12142/3//COMMIT_MSG@9
PS3, Line 9: (usually because a user has
: hit Control-C)
> Not that important, but is this really true? Some other cancellation situat
Thanks good point, "usually" is definitely wrong.
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.h@239
PS3, Line 239: Execution backend.
> Thrift execution backend?
Done
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h@294
PS1, Line 294: Status DoCancelQueryFInstancesRrpcWithRetry(
> I believe our current clang-format rules will tell you to put the symbols n
Done
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.cc@427
PS3, Line 427: auto
> We generally avoid the use of auto as it hurts readability.
Done
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/runtime/coordinator-backend-state.cc@440
PS3, Line 440: DoCancelQueryFInstancesRrpcWithRetry
> Is there a reasonable way to make this generic like DoRpcWithRetry()? We'll
I'm doing retry using a lambda
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/scheduling/query-schedule.h@88
PS3, Line 88: execution backend
> Thrift execution backend?
Done
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.h@41
PS3, Line 41: public:
> Not your code, but while you're here would you mind fixing the spacing in t
Done
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.h@63
PS3, Line 63: /// Cancel any executing fragment instances for the query id specified in 'req'.
> nit: could you move this under ReportExecStatus() to keep all of the rpc ha
Done
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.h@64
PS3, Line 64: void
> virtual
Done
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12142/3/be/src/service/control-service.cc@173
PS3, Line 173: "query"
> Since this is a constant, its not really necessary to Substitute() it.
Done
http://gerrit.cloudera.org:8080/#/c/12142/3/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:
http://gerrit.cloudera.org:8080/#/c/12142/3/common/protobuf/control_service.proto@181
PS3, Line 181: rpc CancelQueryFInstances(CancelQueryFInstancesRequestPB) returns (CancelQueryFInstancesResponsePB);
> Long line.
Done, I added IMPALA-8047 for the clang-format support
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Jan 2019 18:36:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
IMPALA-7468: Port CancelQueryFInstances() to KRPC.
When the Coordinator needs to cancel a query (usually because a user has
hit Control-C), it does this by sending a CancelQueryFInstances message
to each fragment instance. This change switches this code to use KRPC.
Add new protobuf definitions for the messages, and remove the old thrift
definitions. Move the server-side implementation of Cancel() from
ImpalaInternalService to ControlService. Rework the scheduler so
that the FInstanceExecParams always contains the KRPC address of the
fragment executors, this address can then be used if a query is to be
cancelled.
For now keep the KRPC calls to CancelQueryFInstances() as synchronous.
While moving the client-side code, remove the fault injection code that
was inserted with FAULT_INJECTION_SEND_RPC_EXCEPTION and
FAULT_INJECTION_RECV_RPC_EXCEPTION (triggered by running impalad with
--fault_injection_rpc_exception_type=1) as this tickles code in
client-cache.h which is now not used.
TESTING:
Ran all end-to-end tests.
No new tests as test_cancellation.py provides good coverage.
Checked in debugger that DebugAction style fault injection (triggered
from test_cancellation.py) was working correctly.
Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
12 files changed, 164 insertions(+), 105 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12142/2
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Jan 2019 20:54:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/service/control-service.cc@166
PS1, Line 166: class CancelQueryFInstancesResponsePB* response, ::kudu::rpc::RpcContext* rpc_context) {
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Jan 2019 18:21:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
IMPALA-7468: Port CancelQueryFInstances() to KRPC.
When the Coordinator needs to cancel a query (for example because a user
has hit Control-C), it does this by sending a CancelQueryFInstances
message to each fragment instance. This change switches this code to use
KRPC.
Add new protobuf definitions for the messages, and remove the old thrift
definitions. Move the server-side implementation of Cancel() from
ImpalaInternalService to ControlService. Rework the scheduler so
that the FInstanceExecParams always contains the KRPC address of the
fragment executors, this address can then be used if a query is to be
cancelled.
For now keep the KRPC calls to CancelQueryFInstances() as synchronous.
While moving the client-side code, remove the fault injection code that
was inserted with FAULT_INJECTION_SEND_RPC_EXCEPTION and
FAULT_INJECTION_RECV_RPC_EXCEPTION (triggered by running impalad with
--fault_injection_rpc_exception_type=1) as this tickles code in
client-cache.h which is now not used.
TESTING:
Ran all end-to-end tests.
No new tests as test_cancellation.py provides good coverage.
Checked in debugger that DebugAction style fault injection (triggered
from test_cancellation.py) was working correctly.
Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
12 files changed, 192 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12142/4
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1698/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Jan 2019 18:50:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3617/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Jan 2019 21:02:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
IMPALA-7468: Port CancelQueryFInstances() to KRPC.
When the Coordinator needs to cancel a query (usually because a user has
hit Control-C), it does this by sending a CancelQueryFInstances message
to each fragment instance. This change switches this code to use KRPC.
Add new protobuf definitions for the messages, and remove the old thrift
definitions. Move the server-side implementation of Cancel() from
ImpalaInternalService to ControlService. Rework the scheduler so
that the FInstanceExecParams always contains the KRPC address of the
fragment executors, this address can then be used if a query is to be
cancelled.
For now keep the KRPC calls to CancelQueryFInstances() as synchronous.
While moving the client-side code, remove the fault injection code that
was inserted with FAULT_INJECTION_SEND_RPC_EXCEPTION and
FAULT_INJECTION_RECV_RPC_EXCEPTION (triggered by running impalad with
--fault_injection_rpc_exception_type=1) as this tickles code in
client-cache.h which is now not used.
TESTING:
Ran all end-to-end tests.
No new tests as test_cancellation.py provides good coverage.
Checked in debugger that DebugAction style fault injection (triggered
from test_cancellation.py) was working correctly.
Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
12 files changed, 158 insertions(+), 111 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12142/3
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 01:05:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
IMPALA-7468: Port CancelQueryFInstances() to KRPC.
When the Coordinator needs to cancel a query (for example because a user
has hit Control-C), it does this by sending a CancelQueryFInstances
message to each fragment instance. This change switches this code to use
KRPC.
Add new protobuf definitions for the messages, and remove the old thrift
definitions. Move the server-side implementation of Cancel() from
ImpalaInternalService to ControlService. Rework the scheduler so
that the FInstanceExecParams always contains the KRPC address of the
fragment executors, this address can then be used if a query is to be
cancelled.
For now keep the KRPC calls to CancelQueryFInstances() as synchronous.
While moving the client-side code, remove the fault injection code that
was inserted with FAULT_INJECTION_SEND_RPC_EXCEPTION and
FAULT_INJECTION_RECV_RPC_EXCEPTION (triggered by running impalad with
--fault_injection_rpc_exception_type=1) as this tickles code in
client-cache.h which is now not used.
TESTING:
Ran all end-to-end tests.
No new tests as test_cancellation.py provides good coverage.
Checked in debugger that DebugAction style fault injection (triggered
from test_cancellation.py) was working correctly.
Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Reviewed-on: http://gerrit.cloudera.org:8080/12142
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
12 files changed, 192 insertions(+), 139 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
......................................................................
Patch Set 1:
(6 comments)
Thanks for the helpful review
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h@294
PS1, Line 294: Status DoCancelQueryFInstancesRrpcWithRetry(
> Nit: The prevailing style seems to place reference ('&') and pointer ('*')
Up to now I have just been using what the clang-format rules dictate. In clang-format terms I think you are saying we should use 'DerivePointerAlignment: true' (which means: "analyze the formatted file for the most common alignment of & and *. Pointer and reference alignment styles are going to be updated according to the preferences found in the file. PointerAlignment is then used only as fallback"), is that the standard practice for Impala?
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h@104
PS1, Line 104: const TNetworkAddress& krpc_host, int per_fragment_instance_idx,
> Nit: I wonder if there are trailing whitespaces in these lines? I don't und
There is no trailing whitespace I could find.
I think this formatting is the result of the clang-format parameter: ConstructorInitializerAllOnOneLineOrOnePerLine: true
which is splitting the initializers to one-per-line.
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.h@433
PS1, Line 433: void ComputeFragmentExecParams(const BackendConfig executor_config,
> Nit: Please retain existing placement of '&' and '*'.
Thanks for catching my not using a reference for executor_config.
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.cc@333
PS1, Line 333: ComputeFragmentExecParams(executor_config, plan_exec_info,
> Nit: Trailing spaces?
Yes there is something wrong here and I have reformatted
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@163
PS1, Line 163: optional UniqueIdPB query_id = 1;
> Don't understand how the query id can be optional...
OK you made me think!
I had just copied what other people had done in implementing krpc. But now I have investigated further and found that many people argue that having required fields is a bad idea, and the syntax is even removed in future protobuf implementations, see for example https://stackoverflow.com/questions/31801257/why-required-and-optional-is-removed-in-protocol-buffers-3
Overall this makes sense to me, so I think optional is OK here, are you persuaded?
http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@179
PS1, Line 179: // Cancellation is asynchronous.
> Commit message says cancellation is synchronous. Do you want to change this
When the coordinator cancels a query it sends CancelQueryFInstancesRequestPB messages to the hosts executing fragment instances for the query. These calls are synchronous, that is the coordinator waits for a response to each CancelQueryFInstances call before making the next one. This is what the commit message means by 'For now keep the KRPC calls to CancelQueryFInstances() as synchronous'. When a cancellation occurs on an impalad executing a fragment instance then the cancellation is implemented by setting flags saying the query is cancelled in various data structures, and by waking various sleeping threads. This should be enough to stop the query executing, but the CancelQueryFInstances call returns without doing any checking that all query execution has stopped. So in this sense cancel is asynchronous. I updated the comment to make this clearer.
--
To view, visit http://gerrit.cloudera.org:8080/12142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Jan 2019 00:33:30 +0000
Gerrit-HasComments: Yes