[Impala-ASF-CR] IMPALA-6728: Always use Kudu based kinit if FLAGS use krpc=true

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9797

to look at the new patch set (#3).

Change subject: IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true
..

IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true

We rely on the KPRC logic to do the Kerberos authentication
when KRPC is enabled. Therefore, when FLAGS_ues_krpc=true,
we must always call kudu::security::InitKerberosForServer()
to initialize the Kerberos related logic. This change makes
Impala ignore FLAGS_use_kudu_kinit=false when FLAGS_use_krpc=true.

Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
---
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/mini-kdc-wrapper.cc
M be/src/testutil/mini-kdc-wrapper.h
7 files changed, 146 insertions(+), 113 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9797/3
--
To view, visit http://gerrit.cloudera.org:8080/9797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
Gerrit-Change-Number: 9797
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6728: Always use Kudu based kinit if FLAGS use krpc=true

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9797

to look at the new patch set (#4).

Change subject: IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true
..

IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true

We rely on the KPRC logic to do the Kerberos authentication
when KRPC is enabled. Therefore, when FLAGS_ues_krpc=true,
we must always call kudu::security::InitKerberosForServer()
to initialize the Kerberos related logic. This change makes
Impala ignore FLAGS_use_kudu_kinit=false when FLAGS_use_krpc=true.

Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
---
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/mini-kdc-wrapper.cc
M be/src/testutil/mini-kdc-wrapper.h
7 files changed, 149 insertions(+), 113 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9797/4
--
To view, visit http://gerrit.cloudera.org:8080/9797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
Gerrit-Change-Number: 9797
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6728: Always use Kudu based kinit if FLAGS use krpc=true

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9797 )

Change subject: IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true
..


Patch Set 4: Code-Review+2

Carry +2. Fixed clang-tidy errors.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
Gerrit-Change-Number: 9797
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 27 Mar 2018 05:51:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6728: Always use Kudu based kinit if FLAGS use krpc=true

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9797 )

Change subject: IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true
..


Patch Set 3: Code-Review+2

Carry +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
Gerrit-Change-Number: 9797
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 27 Mar 2018 00:32:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 5:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.cc
File be/src/runtime/krpc-data-stream-recvr.cc:

http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.cc@457
PS3, Line 457: kudu::Slice tuple_offsets;
 : kudu::Slice tuple_data;
 : int64_t batch_size;
 : status = UnpackRequest(ctx->request, ctx->rpc_context, 
_offsets,
 : _data, _size);
 : // Reply with error status if the entry cannot be unpacked.
 : if (UNLIKELY(!status.ok())) {
 :   DataStreamService::RespondAndReleaseRpc(status, 
ctx->response, ctx->rpc_context,
 :   recvr_->deferred_rpc_tracker());
 :   DequeueDeferredRpc();
 :   return;
 : }
 :
 :
> Right, but it also gives the serialized size components. What I meant was:
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc
File be/src/runtime/krpc-data-stream-recvr.cc:

http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@117
PS4, Line 117: doe
> does
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@121
PS4, Line 121: doe
> does
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@207
PS4, Line 207: l_has_deferred_rpcs_timer_'.
> how about calling it: has_deferred_rpcs_start_time_ns_?
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@318
PS4, Line 318:   deferred_rpcs_.pop();
> DCHECK_NE(has_deferred_rpcs_start_time_ns, 0)?
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@320
PS4, Line 320: ed_rpcs_start_time_ns_, 0
> how about calling this: total_has_deferred_rpcs_time_ to make it clearer th
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@642
PS4, Line 642: chWork()
> sometimes we have "counter_" and sometimes not. Let's try to be consistent
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@646
PS4, Line 646: rious
> okay to ignore, but I think it'd be good to call these timer_ (but calling
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@673
PS4, Line 673: total_deferred_rpcs_count
> as mentioned earlier, how about total_has_deferred_rpcs_time_ to distinguis
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@674
PS4, Line 674:  "TotalRPCsDeferred",
> TotalHasDeferredRPCsTime
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-sender.h
File be/src/runtime/krpc-data-stream-sender.h:

http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-sender.h@183
PS4, Line 183: m
> maybe add: but not meaningful by itself, and therefore not placed in a prof
Done


http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:

http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-sender.cc@405
PS4, Line 405: Substitute("TransmitData() to $0 failed", 
TNetworkAddressToString(address_));
> maybe set rpc_start_time_ns_ to 0 here and then DCHECK at that it's not 0 a
Set to 0 in MarkDone().


http://gerrit.cloudera.org:8080/#/c/9690/4/common/protobuf/data_stream_service.proto
File common/protobuf/data_stream_service.proto:

http://gerrit.cloudera.org:8080/#/c/9690/4/common/protobuf/data_stream_service.proto@53
PS4, Line 53: receivin
> let's clarify that, since it could be intepretted as the data stream recvr
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Tue, 27 Mar 2018 00:46:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6728: Always use Kudu based kinit if FLAGS use krpc=true

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9797 )

Change subject: IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9797/2/be/src/rpc/rpc-mgr-kerberized-test.cc
File be/src/rpc/rpc-mgr-kerberized-test.cc:

http://gerrit.cloudera.org:8080/#/c/9797/2/be/src/rpc/rpc-mgr-kerberized-test.cc@57
PS2, Line 57: // TODO: IMPALA-6477: This test breaks on CentOS 6.4. Re-enable 
after a fix.
This can be removed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
Gerrit-Change-Number: 9797
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 27 Mar 2018 00:32:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#5). ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..

IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

This change implements a couple of improvements to the profiles of
KrpcDataStreamRecvr and KrpcDataStreamSender:

- track pending number of deferred row batches over time in KrpcDataStreamRecvr
- track the number of bytes dequeued over time in KrpcDataStreamRecvr
- track the total time deferred RPCs queues are not empty
- track the number of bytes sent from KrpcDataStreamSender over time
- track the total amount of time spent in KrpcDataStreamSender, including time
  spent waiting for RPC completion.

Sample profile of an Exchange node instance:

  EXCHANGE_NODE (id=21):(Total: 2s284ms, non-child: 64.926ms, % 
non-child: 2.84%)
 - ConvertRowBatchTime: 44.380ms
 - PeakMemoryUsage: 124.04 KB (127021)
 - RowsReturned: 287.51K (287514)
 - RowsReturnedRate: 125.88 K/sec
Buffer pool:
   - AllocTime: 1.109ms
   - CumulativeAllocationBytes: 10.96 MB (11493376)
   - CumulativeAllocations: 562 (562)
   - PeakReservation: 112.00 KB (114688)
   - PeakUnpinnedBytes: 0
   - PeakUsedReservation: 112.00 KB (114688)
   - ReadIoBytes: 0
   - ReadIoOps: 0 (0)
   - ReadIoWaitTime: 0.000ns
   - WriteIoBytes: 0
   - WriteIoOps: 0 (0)
   - WriteIoWaitTime: 0.000ns
Dequeue:
  BytesDequeued(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 700.00 KB, 2.00 MB, 3.49 
MB, 4.39 MB, 5.86 MB, 6.85 MB
   - FirstBatchWaitTime: 0.000ns
   - TotalBytesDequeued: 6.85 MB (7187850)
   - TotalGetBatchTime: 2s237ms
 - DataWaitTime: 2s219ms
Enqueue:
  BytesReceived(500.000ms): 0, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 328.73 KB, 963.79 KB, 1.64 MB, 2.09 MB, 2.76 MB, 3.23 MB
  DeferredQueueSize(500.000ms): 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0
   - DispatchTime: (Avg: 108.593us ; Min: 30.525us ; Max: 1.524ms ; 
Number of samples: 281)
   - DeserializeRowBatchTime: 8.395ms
   - TotalBatchesEnqueued: 281 (281)
   - TotalBatchesReceived: 281 (281)
   - TotalBytesReceived: 3.23 MB (3387144)
   - TotalEarlySenders: 0 (0)
   - TotalEosReceived: 1 (1)
   - TotalHasDeferredRPCsTime: 15s446ms
   - TotalRPCsDeferred: 38 (38)

Sample sender's profile:

KrpcDataStreamSender (dst_id=21):(Total: 17s923ms, non-child: 
604.494ms, % non-child: 3.37%)
  BytesSent(500.000ms): 0, 0, 0, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 46.54 KB, 46.54 KB, 46.54 KB, 58.31 KB, 58.31 KB, 58.31 KB, 58.31 KB, 58.31 
KB, 58.31 KB, 58.31 KB, 974.44 KB, 2.82 MB, 4.93 MB, 6.27 MB, 8.28 MB, 9.69 MB
   - EosSent: 3 (3)
   - NetworkThroughput: 4.61 MB/sec
   - PeakMemoryUsage: 22.57 KB (23112)
   - RowsSent: 287.51K (287514)
   - RpcFailure: 0 (0)
   - RpcRetry: 0 (0)
   - SerializeBatchTime: 329.162ms
   - TotalBytesSent: 9.69 MB (10161432)
   - UncompressedRowBatchSize: 20.56 MB (21563550)

Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
---
M be/src/kudu/rpc/rpc_context.cc
M be/src/kudu/rpc/rpc_context.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/runtime-state.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M common/protobuf/data_stream_service.proto
12 files changed, 288 insertions(+), 138 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9690/5
--
To view, visit http://gerrit.cloudera.org:8080/9690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Hello Lars Volker, Mostafa Mokhtar, Dan Hecht, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9690

to look at the new patch set (#7).

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..

IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

This change implements a couple of improvements to the profiles of
KrpcDataStreamRecvr and KrpcDataStreamSender:

- track pending number of deferred row batches over time in KrpcDataStreamRecvr
- track the number of bytes dequeued over time in KrpcDataStreamRecvr
- track the total time deferred RPCs queues are not empty
- track the number of bytes sent from KrpcDataStreamSender over time
- track the total amount of time spent in KrpcDataStreamSender, including time
  spent waiting for RPC completion.

Sample profile of an Exchange node instance:

  EXCHANGE_NODE (id=21):(Total: 2s284ms, non-child: 64.926ms, % 
non-child: 2.84%)
 - ConvertRowBatchTime: 44.380ms
 - PeakMemoryUsage: 124.04 KB (127021)
 - RowsReturned: 287.51K (287514)
 - RowsReturnedRate: 125.88 K/sec
Buffer pool:
   - AllocTime: 1.109ms
   - CumulativeAllocationBytes: 10.96 MB (11493376)
   - CumulativeAllocations: 562 (562)
   - PeakReservation: 112.00 KB (114688)
   - PeakUnpinnedBytes: 0
   - PeakUsedReservation: 112.00 KB (114688)
   - ReadIoBytes: 0
   - ReadIoOps: 0 (0)
   - ReadIoWaitTime: 0.000ns
   - WriteIoBytes: 0
   - WriteIoOps: 0 (0)
   - WriteIoWaitTime: 0.000ns
Dequeue:
  BytesDequeued(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 700.00 KB, 2.00 MB, 3.49 
MB, 4.39 MB, 5.86 MB, 6.85 MB
   - FirstBatchWaitTime: 0.000ns
   - TotalBytesDequeued: 6.85 MB (7187850)
   - TotalGetBatchTime: 2s237ms
 - DataWaitTime: 2s219ms
Enqueue:
  BytesReceived(500.000ms): 0, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 328.73 KB, 963.79 KB, 1.64 MB, 2.09 MB, 2.76 MB, 3.23 MB
  DeferredQueueSize(500.000ms): 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0
   - DispatchTime: (Avg: 108.593us ; Min: 30.525us ; Max: 1.524ms ; 
Number of samples: 281)
   - DeserializeRowBatchTime: 8.395ms
   - TotalBatchesEnqueued: 281 (281)
   - TotalBatchesReceived: 281 (281)
   - TotalBytesReceived: 3.23 MB (3387144)
   - TotalEarlySenders: 0 (0)
   - TotalEosReceived: 1 (1)
   - TotalHasDeferredRPCsTime: 15s446ms
   - TotalRPCsDeferred: 38 (38)

Sample sender's profile:

KrpcDataStreamSender (dst_id=21):(Total: 17s923ms, non-child: 
604.494ms, % non-child: 3.37%)
  BytesSent(500.000ms): 0, 0, 0, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 46.54 KB, 46.54 KB, 46.54 KB, 58.31 KB, 58.31 KB, 58.31 KB, 58.31 KB, 58.31 
KB, 58.31 KB, 58.31 KB, 974.44 KB, 2.82 MB, 4.93 MB, 6.27 MB, 8.28 MB, 9.69 MB
   - EosSent: 3 (3)
   - NetworkThroughput: 4.61 MB/sec
   - PeakMemoryUsage: 22.57 KB (23112)
   - RowsSent: 287.51K (287514)
   - RpcFailure: 0 (0)
   - RpcRetry: 0 (0)
   - SerializeBatchTime: 329.162ms
   - TotalBytesSent: 9.69 MB (10161432)
   - UncompressedRowBatchSize: 20.56 MB (21563550)

Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/runtime-state.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M common/protobuf/data_stream_service.proto
10 files changed, 281 insertions(+), 138 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9690/7
--
To view, visit http://gerrit.cloudera.org:8080/9690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: 

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Hello Lars Volker, Mostafa Mokhtar, Dan Hecht, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9690

to look at the new patch set (#8).

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..

IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

This change implements a couple of improvements to the profiles of
KrpcDataStreamRecvr and KrpcDataStreamSender:

- track pending number of deferred row batches over time in KrpcDataStreamRecvr
- track the number of bytes dequeued over time in KrpcDataStreamRecvr
- track the total time deferred RPCs queues are not empty
- track the number of bytes sent from KrpcDataStreamSender over time
- track the total amount of time spent in KrpcDataStreamSender, including time
  spent waiting for RPC completion.

Sample profile of an Exchange node instance:

  EXCHANGE_NODE (id=21):(Total: 2s284ms, non-child: 64.926ms, % 
non-child: 2.84%)
 - ConvertRowBatchTime: 44.380ms
 - PeakMemoryUsage: 124.04 KB (127021)
 - RowsReturned: 287.51K (287514)
 - RowsReturnedRate: 125.88 K/sec
Buffer pool:
   - AllocTime: 1.109ms
   - CumulativeAllocationBytes: 10.96 MB (11493376)
   - CumulativeAllocations: 562 (562)
   - PeakReservation: 112.00 KB (114688)
   - PeakUnpinnedBytes: 0
   - PeakUsedReservation: 112.00 KB (114688)
   - ReadIoBytes: 0
   - ReadIoOps: 0 (0)
   - ReadIoWaitTime: 0.000ns
   - WriteIoBytes: 0
   - WriteIoOps: 0 (0)
   - WriteIoWaitTime: 0.000ns
Dequeue:
  BytesDequeued(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 700.00 KB, 2.00 MB, 3.49 
MB, 4.39 MB, 5.86 MB, 6.85 MB
   - FirstBatchWaitTime: 0.000ns
   - TotalBytesDequeued: 6.85 MB (7187850)
   - TotalGetBatchTime: 2s237ms
 - DataWaitTime: 2s219ms
Enqueue:
  BytesReceived(500.000ms): 0, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 328.73 KB, 963.79 KB, 1.64 MB, 2.09 MB, 2.76 MB, 3.23 MB
  DeferredQueueSize(500.000ms): 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0
   - DispatchTime: (Avg: 108.593us ; Min: 30.525us ; Max: 1.524ms ; 
Number of samples: 281)
   - DeserializeRowBatchTime: 8.395ms
   - TotalBatchesEnqueued: 281 (281)
   - TotalBatchesReceived: 281 (281)
   - TotalBytesReceived: 3.23 MB (3387144)
   - TotalEarlySenders: 0 (0)
   - TotalEosReceived: 1 (1)
   - TotalHasDeferredRPCsTime: 15s446ms
   - TotalRPCsDeferred: 38 (38)

Sample sender's profile:

KrpcDataStreamSender (dst_id=21):(Total: 17s923ms, non-child: 
604.494ms, % non-child: 3.37%)
  BytesSent(500.000ms): 0, 0, 0, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 46.54 KB, 46.54 KB, 46.54 KB, 58.31 KB, 58.31 KB, 58.31 KB, 58.31 KB, 58.31 
KB, 58.31 KB, 58.31 KB, 974.44 KB, 2.82 MB, 4.93 MB, 6.27 MB, 8.28 MB, 9.69 MB
   - EosSent: 3 (3)
   - NetworkThroughput: 4.61 MB/sec
   - PeakMemoryUsage: 22.57 KB (23112)
   - RowsSent: 287.51K (287514)
   - RpcFailure: 0 (0)
   - RpcRetry: 0 (0)
   - SerializeBatchTime: 329.162ms
   - TotalBytesSent: 9.69 MB (10161432)
   - UncompressedRowBatchSize: 20.56 MB (21563550)

Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/runtime-state.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M common/protobuf/data_stream_service.proto
10 files changed, 281 insertions(+), 138 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9690/8
--
To view, visit http://gerrit.cloudera.org:8080/9690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: 

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 8: Code-Review+2

Fix clang-tidy error. Rebase. Carry +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Wed, 28 Mar 2018 07:50:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Hello Lars Volker, Mostafa Mokhtar, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9690

to look at the new patch set (#6).

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..

IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

This change implements a couple of improvements to the profiles of
KrpcDataStreamRecvr and KrpcDataStreamSender:

- track pending number of deferred row batches over time in KrpcDataStreamRecvr
- track the number of bytes dequeued over time in KrpcDataStreamRecvr
- track the total time deferred RPCs queues are not empty
- track the number of bytes sent from KrpcDataStreamSender over time
- track the total amount of time spent in KrpcDataStreamSender, including time
  spent waiting for RPC completion.

Sample profile of an Exchange node instance:

  EXCHANGE_NODE (id=21):(Total: 2s284ms, non-child: 64.926ms, % 
non-child: 2.84%)
 - ConvertRowBatchTime: 44.380ms
 - PeakMemoryUsage: 124.04 KB (127021)
 - RowsReturned: 287.51K (287514)
 - RowsReturnedRate: 125.88 K/sec
Buffer pool:
   - AllocTime: 1.109ms
   - CumulativeAllocationBytes: 10.96 MB (11493376)
   - CumulativeAllocations: 562 (562)
   - PeakReservation: 112.00 KB (114688)
   - PeakUnpinnedBytes: 0
   - PeakUsedReservation: 112.00 KB (114688)
   - ReadIoBytes: 0
   - ReadIoOps: 0 (0)
   - ReadIoWaitTime: 0.000ns
   - WriteIoBytes: 0
   - WriteIoOps: 0 (0)
   - WriteIoWaitTime: 0.000ns
Dequeue:
  BytesDequeued(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 700.00 KB, 2.00 MB, 3.49 
MB, 4.39 MB, 5.86 MB, 6.85 MB
   - FirstBatchWaitTime: 0.000ns
   - TotalBytesDequeued: 6.85 MB (7187850)
   - TotalGetBatchTime: 2s237ms
 - DataWaitTime: 2s219ms
Enqueue:
  BytesReceived(500.000ms): 0, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 KB, 23.36 
KB, 23.36 KB, 23.36 KB, 328.73 KB, 963.79 KB, 1.64 MB, 2.09 MB, 2.76 MB, 3.23 MB
  DeferredQueueSize(500.000ms): 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0
   - DispatchTime: (Avg: 108.593us ; Min: 30.525us ; Max: 1.524ms ; 
Number of samples: 281)
   - DeserializeRowBatchTime: 8.395ms
   - TotalBatchesEnqueued: 281 (281)
   - TotalBatchesReceived: 281 (281)
   - TotalBytesReceived: 3.23 MB (3387144)
   - TotalEarlySenders: 0 (0)
   - TotalEosReceived: 1 (1)
   - TotalHasDeferredRPCsTime: 15s446ms
   - TotalRPCsDeferred: 38 (38)

Sample sender's profile:

KrpcDataStreamSender (dst_id=21):(Total: 17s923ms, non-child: 
604.494ms, % non-child: 3.37%)
  BytesSent(500.000ms): 0, 0, 0, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 KB, 34.78 
KB, 46.54 KB, 46.54 KB, 46.54 KB, 58.31 KB, 58.31 KB, 58.31 KB, 58.31 KB, 58.31 
KB, 58.31 KB, 58.31 KB, 974.44 KB, 2.82 MB, 4.93 MB, 6.27 MB, 8.28 MB, 9.69 MB
   - EosSent: 3 (3)
   - NetworkThroughput: 4.61 MB/sec
   - PeakMemoryUsage: 22.57 KB (23112)
   - RowsSent: 287.51K (287514)
   - RpcFailure: 0 (0)
   - RpcRetry: 0 (0)
   - SerializeBatchTime: 329.162ms
   - TotalBytesSent: 9.69 MB (10161432)
   - UncompressedRowBatchSize: 20.56 MB (21563550)

Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/runtime-state.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M common/protobuf/data_stream_service.proto
10 files changed, 281 insertions(+), 138 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9690/6
--
To view, visit http://gerrit.cloudera.org:8080/9690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: 

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 6: Code-Review+2

Rebase. Carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Tue, 27 Mar 2018 17:00:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] Revert "IMPALA-6747: Automate diagnostics collection."

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9872 )

Change subject: Revert "IMPALA-6747: Automate diagnostics collection."
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I221ede9d5eb4d89ea20992cc27a8284803af3223
Gerrit-Change-Number: 9872
Gerrit-PatchSet: 1
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Fri, 30 Mar 2018 21:11:49 +
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2385: Fix typo in KinitContext::DoRenewal()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9842 )

Change subject: KUDU-2385: Fix typo in KinitContext::DoRenewal()
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
Gerrit-Change-Number: 9842
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 28 Mar 2018 21:44:21 +
Gerrit-HasComments: No

[Impala-ASF-CR] Do clean as part of "bootstrap development.sh"

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9841 )

Change subject: Do clean as part of "bootstrap_development.sh"
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I238528ca3938200f0750505a62113630290e8c96
Gerrit-Change-Number: 9841
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 28 Mar 2018 21:38:17 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6759: align stress test memory estimation parse pattern

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9844 )

Change subject: IMPALA-6759: align stress test memory estimation parse pattern
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08976f261582b379696fd0e81bc060577e552309
Gerrit-Change-Number: 9844
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 28 Mar 2018 22:22:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 4:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-mgr.cc@201
PS3, Line 201:   TUniqueId finst_id;
> its impossible to know what this is all about until you read the other code
Done


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.h
File be/src/runtime/krpc-data-stream-recvr.h:

http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.h@171
PS3, Line 171: num_deferred_rpcs()
> should this be num_deferred_rpcs? We've been referring to these as "deferre
Done


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.h@245
PS3, Line 245:  Following counters belong to 'dequeue_profile
> is that accurate? what does "deferred row batches" have to do with sampling
Oops.. copied and pasted from elsewhere.


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.h@254
PS3, Line 254:   RuntimeProfile::Counter* queue_get_batch_time_;
> then it should be in this section, right?
Done


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.h@255
PS3, Line 255:
> then why have data_wait_time_?
Inactive time is not shown in the profile. It's only used for computing the 
"non-child" time of the exchange node. I find "non-child" time inaccurate but I 
will refrain from fixing it in this change.


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.h@262
PS3, Line 262:   /// 

 :   /// Following counters belong to 'enqueue_pro
> why is this in the "dequeue" profile?  seems more enqueue side, no?
Done


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.h@294
PS3, Line 294: RuntimeProfile::Counter* total_deferred_rpcs_time_;
This is converted to tracking the cumulative time in which 'deferred_rpcs_' is 
not empty in the new patch as the average tends to be skewed outlier (e.g. the 
amount of time between when a RPC arrives and when the parent of exchange node 
actually calls GetBatch()).


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.cc
File be/src/runtime/krpc-data-stream-recvr.cc:

http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.cc@212
PS3, Line 212: parent_recvr), num_remaining_senders_(num_senders) {
> nit: would be nice to order these in the same order of the comment
Done


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.cc@457
PS3, Line 457:   DataStreamService::RespondAndReleaseRpc(status, 
ctx->response, ctx->rpc_context,
 :   recvr_->deferred_rpc_tracker());
 :   DequeueDeferredRpc();
 :   return;
 : }
 :
 : // Stops if inserting the batch causes us to go over the 
limit.
 : // Put 'ctx' back on the queue.
 : if (!CanEnqueue(batch_size)) {
 :   ctx.swap(deferred_rpcs_.front());
 :   DCHECK(deferred_rpcs_.front().get() != nullptr);
 :   return;
 : }
 :
> why not use UnpackRequest() for this?
Convert to a static function. UnpackRequest() was returning deserialized size 
while this returns the serialized size.


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:

http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-sender.cc@388
PS3, Line 388: _in_flight_batch_)
> but that doesn't include time spent in the krpc service queue, right? so ho
Updated the patch to also include the time in service queue. There is currently 
no easy way to track time spent in the outbound transfer queue.


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-sender.cc@389
PS3, Line 389: int64_t network_time = total_time - 
resp_.receiver_latency_ns();
> why is that? why does total time not include time spent waiting for the rep
In the Thrift based implementation, we are mostly tracking the time to cache 
the send buffer and the serialization time.

PS4 fixes this up by counting the total time as time spent in Send(), 
FlushFinal(), Close(), Open() and Prepare(). Also counted the time spent in 
WaitForRpc() as "inactive time" so as to show the actual overhead of DSS.


http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Hello Lars Volker, Mostafa Mokhtar, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9690

to look at the new patch set (#4).

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..

IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

This change implements a couple of improvements to the profiles of
KrpcDataStreamRecvr and KrpcDataStreamSender:

- track pending number of deferred row batches over time in KrpcDataStreamRecvr
- track the number of bytes dequeued over time in KrpcDataStreamRecvr
- track the min/max/avg of time row batches spent in deferred queue
- track the number of bytes sent from KrpcDataStreamSender over time

Sample profile of an Exchange node instance:

  EXCHANGE_NODE (id=82):(Total: 1s367ms, non-child: 418.734us, % 
non-child: 0.03%)
 - ConvertRowBatchTime: 229.386us
 - PeakMemoryUsage: 54.24 KB (55541)
 - RowsReturned: 1.80K (1800)
 - RowsReturnedRate: 1.32 K/sec
Buffer pool:
   - AllocTime: 8.537us
   - CumulativeAllocationBytes: 48.00 KB (49152)
   - CumulativeAllocations: 4 (4)
   - PeakReservation: 48.00 KB (49152)
   - PeakUnpinnedBytes: 0
   - PeakUsedReservation: 48.00 KB (49152)
   - ReadIoBytes: 0
   - ReadIoOps: 0 (0)
   - ReadIoWaitTime: 0.000ns
   - WriteIoBytes: 0
   - WriteIoOps: 0 (0)
   - WriteIoWaitTime: 0.000ns
Dequeue:
  BytesDequeued(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 36.91 KB, 36.91 KB, 36.91 KB
   - FirstBatchWaitTime: 0.000ns
   - TotalBytesDequeued: 36.91 KB (37800)
   - TotalGetBatchTime: 1s366ms
 - DataWaitTime: 1s366ms
Enqueue:
  BytesReceived(500.000ms): 0, 0, 0, 0, 0, 0, 0, 8.10 KB, 14.24 KB, 
14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 
14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 14.24 KB, 
14.24 KB, 14.24 KB
  DeferredQueueSize(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0
   - DispatchTime: (Avg: 258.755us ; Min: 193.825us ; Max: 
323.688us ; Number of samples: 2)
   - DeserializeRowBatchTime: 60.318us
   - TotalBatchesEnqueued: 2 (2)
   - TotalBatchesReceived: 2 (2)
   - TotalBytesReceived: 14.24 KB (14579)
   - TotalEarlySenders: 0 (0)
   - TotalEosReceived: 1 (1)
   - TotalRPCsDeferralTime: 8s171ms
   - TotalRPCsDeferred: 1 (1)

Sample sender's profile:

KrpcDataStreamSender (dst_id=82):(Total: 9s605ms, non-child: 125.726ms, 
% non-child: 1.31%)
  BytesSent(500.000ms): 0, 0, 0, 0, 24.29 KB, 24.29 KB, 24.29 KB, 24.29 
KB, 24.29 KB, 24.29 KB, 24.29 KB, 24.29 KB, 24.29 KB, 24.29 KB, 24.29 KB, 24.29 
KB, 24.29 KB, 24.29 KB, 24.29 KB, 24.29 KB, 24.29 KB, 30.43 KB, 30.43 KB, 30.43 
KB
   - EosSent: 3 (3)
   - NetworkThroughput: 290.09 KB/sec
   - PeakMemoryUsage: 29.53 KB (30240)
   - RowsReturned: 1.80K (1800)
   - RpcFailure: 0 (0)
   - RpcRetry: 0 (0)
   - SerializeBatchTime: 649.087us
   - TotalBytesSent: 42.71 KB (43737)
   - UncompressedRowBatchSize: 110.74 KB (113400)

Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
---
M be/src/kudu/rpc/rpc_context.cc
M be/src/kudu/rpc/rpc_context.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/runtime-state.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M common/protobuf/data_stream_service.proto
12 files changed, 282 insertions(+), 139 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9690/4
--
To view, visit http://gerrit.cloudera.org:8080/9690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/kudu/rpc/rpc_context.h
File be/src/kudu/rpc/rpc_context.h:

http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/kudu/rpc/rpc_context.h@196
PS4, Line 196: MonoTime GetTimeReceived() const;
Will send out a separate change on Kudu side for this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Sat, 24 Mar 2018 22:55:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6728: Always use Kudu based kinit if FLAGS use krpc=true

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9797


Change subject: IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true
..

IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true

We rely on the KPRC logic to do the Kerberos authentication
when KRPC is enabled. Therefore, when FLAGS_ues_krpc=true,
we must always call kudu::security::InitKerberosForServer()
to initialize the Kerberos related logic. This change makes
Impala ignore FLAGS_use_kudu_kinit=false when FLAGS_use_krpc=true.

Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
---
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/mini-kdc-wrapper.cc
M be/src/testutil/mini-kdc-wrapper.h
7 files changed, 136 insertions(+), 104 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9797/1
--
To view, visit http://gerrit.cloudera.org:8080/9797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
Gerrit-Change-Number: 9797
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[Impala-ASF-CR] Revert "IMPALA-6747: Automate diagnostics collection."

[Michael Ho (Code Review)]

Michael Ho has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9872 )

Change subject: Revert "IMPALA-6747: Automate diagnostics collection."
..

Revert "IMPALA-6747: Automate diagnostics collection."

A couple of things donot work in python2.6
 -- Multiple with statements in the same context
 -- shutil.make_archive()

I need a little more time to test the fix with python2.6.
Meanwhile, reverting this to unblock others. I'll resubmit
the fix when I'm confident that it works with python2.6

This reverts commit 2883c9950026db74240a69ab07e867810b8547b0.

Change-Id: I221ede9d5eb4d89ea20992cc27a8284803af3223
Reviewed-on: http://gerrit.cloudera.org:8080/9872
Reviewed-by: Michael Ho 
Tested-by: Michael Ho 
---
D bin/diagnostics/__init__.py
D bin/diagnostics/collect_diagnostics.py
D bin/diagnostics/collect_shared_libs.sh
M bin/rat_exclude_files.txt
D tests/unittests/test_command.py
5 files changed, 0 insertions(+), 620 deletions(-)

Approvals:
  Michael Ho: Looks good to me, approved; Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I221ede9d5eb4d89ea20992cc27a8284803af3223
Gerrit-Change-Number: 9872
Gerrit-PatchSet: 2
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-6728: Always use Kudu based kinit if FLAGS use krpc=true

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9797 )

Change subject: IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9797/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/9797/1/be/src/rpc/thrift-server-test.cc@106
PS1, Line 106: FLAGS_prinicpal
> FLAGS_principal
Done


http://gerrit.cloudera.org:8080/#/c/9797/1/be/src/rpc/thrift-server-test.cc@112
PS1, Line 112:   FLAGS_be_principal = "";
> how does this work to turn off kerberos?
Impala internally relies on !FLAGS_principal.empty() to infer whether Kerberos 
is enabled. Please see 
https://github.com/apache/impala/blob/master/be/src/util/auth-util.cc#L103-L105


http://gerrit.cloudera.org:8080/#/c/9797/1/be/src/testutil/mini-kdc-wrapper.cc
File be/src/testutil/mini-kdc-wrapper.cc:

http://gerrit.cloudera.org:8080/#/c/9797/1/be/src/testutil/mini-kdc-wrapper.cc@a68
PS1, Line 68:
> related to other question: why don't we need to skip this stuff still when
The goal is to keep this function simple by just being responsible for setting 
up the KDC. It's okay to start KDC but not enable Kerberos in Impala. The 
previous logic to mix in the KerberosSwitch seems a bit unnecessary as this 
kind of FLAGS configuration should be left in the BE tests themselves.


http://gerrit.cloudera.org:8080/#/c/9797/1/be/src/testutil/mini-kdc-wrapper.cc@a89
PS1, Line 89:
:
The initialization of these flags are moved to the BE tests instead.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
Gerrit-Change-Number: 9797
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 26 Mar 2018 20:56:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] KUDU-2374: Add RpcContext::GetTimeReceived()

[Michael Ho (Code Review)]

Michael Ho has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/9807 )

Change subject: KUDU-2374: Add RpcContext::GetTimeReceived()
..


Removed reviewer Kudu Jenkins.
--
To view, visit http://gerrit.cloudera.org:8080/9807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I6b39c7f2ea856eccfdab8c1bb1433829e979ae13
Gerrit-Change-Number: 9807
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] KUDU-2374: Add RpcContext::GetTimeReceived()

[Michael Ho (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

I'd like you to do a code review. Please visit

http://gerrit.cloudera.org:8080/9807

to review the following change.


Change subject: KUDU-2374: Add RpcContext::GetTimeReceived()
..

KUDU-2374: Add RpcContext::GetTimeReceived()

This change adds RpcContext::GetTimeReceived() which returns
the time at which the inbound call associated with the RpcContext
was received. It's helpful to make this accessible to the RPC
handlers for its own book-keeping purpose (e.g. reporting the
average dispatch latency as part of query profile in Impala).

Change-Id: I6b39c7f2ea856eccfdab8c1bb1433829e979ae13
Reviewed-on: http://gerrit.cloudera.org:8080/9796
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M be/src/kudu/rpc/rpc-test-base.h
M be/src/kudu/rpc/rpc_context.cc
M be/src/kudu/rpc/rpc_context.h
3 files changed, 10 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/9807/1
--
To view, visit http://gerrit.cloudera.org:8080/9807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6b39c7f2ea856eccfdab8c1bb1433829e979ae13
Gerrit-Change-Number: 9807
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] KUDU-2374: Add RpcContext::GetTimeReceived()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9807 )

Change subject: KUDU-2374: Add RpcContext::GetTimeReceived()
..


Patch Set 1:

Clean cherry-pick.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b39c7f2ea856eccfdab8c1bb1433829e979ae13
Gerrit-Change-Number: 9807
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Mon, 26 Mar 2018 21:38:40 +
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2374: Add RpcContext::GetTimeReceived()

[Michael Ho (Code Review)]

Michael Ho has removed Todd Lipcon from this change.  ( 
http://gerrit.cloudera.org:8080/9807 )

Change subject: KUDU-2374: Add RpcContext::GetTimeReceived()
..


Removed reviewer Todd Lipcon.
--
To view, visit http://gerrit.cloudera.org:8080/9807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I6b39c7f2ea856eccfdab8c1bb1433829e979ae13
Gerrit-Change-Number: 9807
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[Impala-ASF-CR] IMPALA-6728: Always use Kudu based kinit if FLAGS use krpc=true

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9797

to look at the new patch set (#2).

Change subject: IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true
..

IMPALA-6728: Always use Kudu based kinit if FLAGS_use_krpc=true

We rely on the KPRC logic to do the Kerberos authentication
when KRPC is enabled. Therefore, when FLAGS_ues_krpc=true,
we must always call kudu::security::InitKerberosForServer()
to initialize the Kerberos related logic. This change makes
Impala ignore FLAGS_use_kudu_kinit=false when FLAGS_use_krpc=true.

Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
---
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/mini-kdc-wrapper.cc
M be/src/testutil/mini-kdc-wrapper.h
7 files changed, 146 insertions(+), 112 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9797/2
--
To view, visit http://gerrit.cloudera.org:8080/9797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia7086e5c9b460233e9e957f886141b3e6bba414b
Gerrit-Change-Number: 9797
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9690


Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..

IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

This change implements a couple of improvements to the profiles of
KrpcDataStreamRecvr and KrpcDataStreamSender:

- track the number of deferred row batches over time in KrpcDataStreamRecvr
- track the number of bytes dequeued over time in KrpcDataStreamRecvr
- track the amount of time row batches spent in deferred queue
- track the portion of wait time in KrpcDataStreamRecvr waiting for data to 
arrive from sender
- track the number of bytes sent from KrpcDataStreamSender over time

A sample profile of an Exchange node instance:

EXCHANGE_NODE (id=33):(Total: 12s359ms, non-child: 86.379ms, % 
non-child: 0.70%)
   - ConvertRowBatchTime: 32.374ms
   - PeakMemoryUsage: 158.90 KB (162710)
   - RowsReturned: 177.37K (177374)
   - RowsReturnedRate: 14.35 K/sec
  Buffer pool:
 - AllocTime: 1.861ms
 - CumulativeAllocationBytes: 8.83 MB (9256960)
 - CumulativeAllocations: 568 (568)
 - PeakReservation: 128.00 KB (131072)
 - PeakUnpinnedBytes: 0
 - PeakUsedReservation: 128.00 KB (131072)
 - ReadIoBytes: 0
 - ReadIoOps: 0 (0)
 - ReadIoWaitTime: 0.000ns
 - WriteIoBytes: 0
 - WriteIoOps: 0 (0)
 - WriteIoWaitTime: 0.000ns
  Dequeue:
BytesDequeued(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
1.24 MB, 1.61 MB, 1.89 MB, 2.07 MB, 2.25 MB, 2.50 MB, 2.78 MB, 3.10 MB, 3.38 
MB, 3.66 MB, 3.84 MB, 4.26 MB, 4.64 MB, 4.90 MB, 5.12 MB, 5.48 MB, 5.78 MB, 
6.01 MB, 6.33 MB, 6.59 MB, 6.64 MB, 6.77 MB, 6.84 MB, 6.95 MB, 7.05 MB
 - FirstBatchArrivalWaitTime: 0.000ns
 - TotalBytesDequeued: 7.10 MB (7449708)
 - TotalEosReceived: 3 (3)
 - TotalGetBatchTime: 12s324ms
   - DataWaitTime: 12s273ms
 - ProducerWaitTime: 12s175ms
  Enqueue:
BytesReceived(500.000ms): 0, 0, 0, 0, 0, 0, 0, 30.56 KB, 40.90 KB, 
40.90 KB, 40.90 KB, 40.90 KB, 40.90 KB, 40.90 KB, 502.04 KB, 655.77 KB, 768.57 
KB, 840.35 KB, 912.10 KB, 1014.75 KB, 1.10 MB, 1.23 MB, 1.34 MB, 1.45 MB, 1.52 
MB, 1.69 MB, 1.84 MB, 1.94 MB, 2.03 MB, 2.17 MB, 2.29 MB, 2.38 MB, 2.51 MB, 
2.61 MB, 2.63 MB, 2.68 MB, 2.71 MB, 2.75 MB, 2.79 MB
DeferredBatches(500.000ms): 0, 0, 0, 0, 0, 0, 0, 2, 3, 3, 3, 3, 3, 
3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
 - DeferredQueueTime: 9s775ms
 - DeserializeRowBatchTime: 9.351ms
 - TotalBatchesDeferred: 35 (35)
 - TotalBatchesEnqueued: 284 (284)
 - TotalBatchesReceived: 284 (284)
 - TotalBytesReceived: 2.82 MB (2954847)
 - TotalEarlySenders: 0 (0)

A sample profile a KrpcDataStreamSender's profile:

KrpcDataStreamSender (dst_id=49):(Total: 204.228ms, non-child: 
204.228ms, % non-child: 100.00%)
  BytesSent(1s000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 40.40 KB, 
46.15 KB, 46.15 KB, 46.15 KB, 46.15 KB, 46.15 KB, 46.15 KB, 775.45 KB, 861.84 
KB, 925.29 KB, 994.57 KB, 1017.76 KB, 1.06 MB, 1.06 MB, 1.11 MB, 1.22 MB, 1.23 
MB, 1.29 MB, 1.34 MB, 1.39 MB, 1.46 MB, 1.52 MB, 1.64 MB, 1.70 MB, 1.76 MB, 
1.82 MB, 1.87 MB, 1.95 MB, 2.11 MB, 2.24 MB, 2.36 MB, 2.49 MB, 2.61 MB, 2.69 
MB, 2.81 MB, 2.96 MB, 3.07 MB, 3.26 MB, 3.53 MB, 3.74 MB, 3.96 MB
   - EosSent: 3 (3)
   - OverallThroughput: 20.54 MB/sec
   - PeakMemoryUsage: 106.55 KB (109104)
   - RowsReturned: 178.80K (178799)
   - RpcFailure: 0 (0)
   - RpcRetry: 0 (0)
   - SerializeBatchTime: 136.159ms
   - TotalBytesSent: 4.19 MB (4398283)
   - UncompressedRowBatchSize: 8.53 MB (8939950)

Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
---
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
5 files changed, 144 insertions(+), 74 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9690/1
--
To view, visit http://gerrit.cloudera.org:8080/9690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[Impala-ASF-CR] IMPALA-6691: KRPC w/ kerberos fails on SLES11

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9696 )

Change subject: IMPALA-6691: KRPC w/ kerberos fails on SLES11
..


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9696/3/be/src/kudu/rpc/messenger.cc
File be/src/kudu/rpc/messenger.cc:

http://gerrit.cloudera.org:8080/#/c/9696/3/be/src/kudu/rpc/messenger.cc@290
PS3, Line 290:   LOG(WARNING) << "Omitting Kerberos pre-flight check. 
Connection negotiations may fail "
nit: indent wrong



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cc7f0702f605fca02a2ff5d3d2735e6e080668
Gerrit-Change-Number: 9696
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 21 Mar 2018 16:58:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6691: KRPC w/ kerberos fails on SLES11

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9696 )

Change subject: IMPALA-6691: KRPC w/ kerberos fails on SLES11
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9696/1/be/src/kudu/rpc/messenger.cc
File be/src/kudu/rpc/messenger.cc:

http://gerrit.cloudera.org:8080/#/c/9696/1/be/src/kudu/rpc/messenger.cc@284
PS1, Line 284:   if (!keytab_file_.empty()) {
> I spent some time trying to disable it only for a specific kerberos version
Were you able to test this change on SLES 11 ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cc7f0702f605fca02a2ff5d3d2735e6e080668
Gerrit-Change-Number: 9696
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Sat, 17 Mar 2018 00:17:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6713: Fix format string error in Sorter

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9740


Change subject: IMPALA-6713: Fix format string error in Sorter
..

IMPALA-6713: Fix format string error in Sorter

This change fixes a format string which incorrectly used
$2 instead of $1 when there are only two arguments to the
format string.

Change-Id: Icdaa781ced755c896cdc9a1fff690811a59e0492
---
M be/src/runtime/sorter.cc
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/9740/1
--
To view, visit http://gerrit.cloudera.org:8080/9740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icdaa781ced755c896cdc9a1fff690811a59e0492
Gerrit-Change-Number: 9740
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9690/2/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9690/2/be/src/runtime/krpc-data-stream-mgr.cc@201
PS2, Line 201: response->set_queue_time(MonotonicNanos());
This may warrant a comment as this is not obvious. We store the start time when 
a RPC first made to KrpcDataStreamMgr and subtract from this stored value in 
DataStreamService::RespondRpc*() to compute the actual queue time.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Wed, 21 Mar 2018 17:19:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6691: KRPC w/ kerberos fails on SLES11

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9696 )

Change subject: IMPALA-6691: KRPC w/ kerberos fails on SLES11
..


Patch Set 2: Code-Review+1

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9696/2/be/CMakeLists.txt
File be/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9696/2/be/CMakeLists.txt@341
PS2, Line 341: krb5_get_init_creds_opt_set_fast_ccache_name
I manually confirmed that this function is not defined in the source code 
https://github.com/krb5/krb5/tree/krb5-1.6/src but defined in 
https://github.com/krb5/krb5/tree/krb5-1.7/src.

It'd be nice to do an objdump of the kerberos shared library on vanilla SLES11 
and Centos6.4 to confirm they match our expectation.


http://gerrit.cloudera.org:8080/#/c/9696/2/be/src/kudu/rpc/messenger.cc
File be/src/kudu/rpc/messenger.cc:

http://gerrit.cloudera.org:8080/#/c/9696/2/be/src/kudu/rpc/messenger.cc@279
PS2, Line 279: Since we support SLES11, we omit
We omit calling ...


http://gerrit.cloudera.org:8080/#/c/9696/2/be/src/kudu/rpc/messenger.cc@282
PS2, Line 282: omiting
nit: omitting


http://gerrit.cloudera.org:8080/#/c/9696/2/be/src/kudu/rpc/messenger.cc@290
PS2, Line 290:
Is it worth adding a quick log statement to indicate that we are skipping 
PreflightCheckGSSAPI() ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cc7f0702f605fca02a2ff5d3d2735e6e080668
Gerrit-Change-Number: 9696
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 21 Mar 2018 05:31:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Hello Lars Volker, Mostafa Mokhtar, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9690

to look at the new patch set (#2).

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..

IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

This change implements a couple of improvements to the profiles of
KrpcDataStreamRecvr and KrpcDataStreamSender:

- track pending number of deferred row batches over time in KrpcDataStreamRecvr
- track the number of bytes dequeued over time in KrpcDataStreamRecvr
- track the min/max/avg of time row batches spent in deferred queue
- track the number of bytes sent from KrpcDataStreamSender over time

Sample profile of an Exchange node instance:

  EXCHANGE_NODE (id=22):(Total: 378.797ms, non-child: 123.042ms, % 
non-child: 32.48%)
 - ConvertRowBatchTime: 70.316ms
 - PeakMemoryUsage: 80.93 KB (82875)
 - RowsReturned: 66.86K (66855)
 - RowsReturnedRate: 176.49 K/sec
Buffer pool:
   - AllocTime: 271.165us
   - CumulativeAllocationBytes: 1.54 MB (1613824)
   - CumulativeAllocations: 132 (132)
   - PeakReservation: 80.00 KB (81920)
   - PeakUnpinnedBytes: 0
   - PeakUsedReservation: 80.00 KB (81920)
   - ReadIoBytes: 0
   - ReadIoOps: 0 (0)
   - ReadIoWaitTime: 0.000ns
   - WriteIoBytes: 0
   - WriteIoOps: 0 (0)
   - WriteIoWaitTime: 0.000ns
Dequeue:
  BytesDequeued(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1.34 MB
   - FirstBatchWaitTime: 0.000ns
   - TotalBytesDequeued: 1.34 MB (1403955)
   - TotalEosReceived: 1 (1)
   - TotalGetBatchTime: 308.074ms
 - DataWaitTime: 255.763ms
Enqueue:
  BytesReceived(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 33.17 KB, 33.17 
KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 
KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 
KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 
KB, 33.17 KB, 1.06 MB
  DeferredBatches(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0
   - DeferredQueueTime: (Avg: 324.328ms ; Min: 316.144us ; Max: 
13s611ms ; Number of samples: 43)
   - DeserializeRowBatchTime: 1.764ms
   - TotalBatchesDeferred: 43 (43)
   - TotalBatchesEnqueued: 66 (66)
   - TotalBatchesReceived: 66 (66)
   - TotalBytesReceived: 1.06 MB (1114795)
   - TotalEarlySenders: 0 (0)

Sample sender's profile:

KrpcDataStreamSender (dst_id=22):(Total: 83.234ms, non-child: 83.234ms, 
% non-child: 100.00%)
  BytesSent(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 16.60 KB, 16.60 KB, 
16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 
16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 
16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 
16.60 KB
   - EosSent: 1 (1)
   - NetworkThroughput: 21.08 MB/sec
   - PeakMemoryUsage: 9.84 KB (10080)
   - RowsReturned: 66.86K (66855)
   - RpcFailure: 0 (0)
   - RpcRetry: 0 (0)
   - SerializeBatchTime: 24.319ms
   - TotalBytesSent: 1.06 MB (1114795)
   - TotalNetworkTime: 50.424ms
   - UncompressedRowBatchSize: 1.34 MB (1403955)

Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/runtime-state.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M common/protobuf/data_stream_service.proto
11 files changed, 227 insertions(+), 129 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9690/2
--
To view, visit http://gerrit.cloudera.org:8080/9690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/9690/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9690/1//COMMIT_MSG@48
PS1, Line 48:- Deserializ
> Following our in-person discussion, this should probably be cummulative.
Actually, after some more thought, this should stay non-cumulative. The reason 
is that this is the key differentiation for telling whether an exchange node is 
stuck because its parent is not consuming from it or if the producers are not 
sending row batches to it. With cumulative value of DeferredBatches, one really 
cannot tell the difference because it would stay the same in either cases.

If we keep the outstanding DeferredBatches count instead, one can tell the 
parent is not consuming from it if both the bytes dequeued and the number of 
outstanding deferred batches stays flat for a period of time. On the other 
hand, if the producer is not producing, one will see the number of deferred 
batches dwindle and the number of bytes dequeued goes up.

In the long run, we ought to keep the row production rate over time per exec 
node to tell where the bottleneck is.


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h
File be/src/runtime/krpc-data-stream-recvr.h:

http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h@240
PS1, Line 240:   /// Following counters belong to 'dequeue_profile_'.
> Should we group them in separate structs? That (or encoding in the names) h
I took the suggestion to add comments to break up the comments of the counters.


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h@263
PS1, Line 263:   RuntimeProfile::Counter* total_eos_received_;
 :
> Should we collect a full histogram instead? You can use a HdrHistogram to t
Looked into using histogram. Seems a bit painful to report the histogram 
periodically as it probably requires custom code in the report exec status path 
to see if an exchange node is in the fragment instance and calls 
RuntimeProfile::AddInfoString() to dump the histogram and it's also a bit hairy 
if you have multiple exchange nodes in a single fragment instance.

An alternate would be to add the histogram at the end of the query but that 
would be not too useful if the query is taking a long time to complete.

That's why I stick with RuntimeProfile::SummaryStatsCounter which seems like a 
reasonable compromise. I am open to suggestion.


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h@292
PS1, Line 292:   /// Summary stats of time which row batches spent in deferred 
queue before
> maybe also add unit here and to the other _time_ members?
I believe all RuntimeProfile::Counters members which measure time don't 
actually have ns_ suffix as they always have nanosecond resolution due to the 
hardcoded resolution in ADD_TIMER(). So, not sure if I want to break the 
convention here and become inconsistent with the rest of the code base.

'wait_time_start_' below needs the 'ns_' suffix as it's using int64_t as a 
timestamp so may help to have the resolution encoded in its name.


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h@297
PS1, Line 297: pace impala
> for uint64 values that hold time, it's nice to encode the units in the name
Done


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc
File be/src/runtime/krpc-data-stream-recvr.cc:

http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc@461
PS1, Line 461:   request->tuple_offsets_sidecar_idx(), 
_offsets).ok())) {
> Should this be a histogram, too?
Please see replies elsewhere.


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc@604
PS1, Line 604: ues = is_merging ?
> Are these the other way round? I think receiver is the enqueue profile, sen
Receiver is the dequeuing side of the 'sender_queue'.

Sender is the enqueuing side of the 'sender_queue'. Sender is always the 
inbound direction.

Comment fixed.


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc@639
PS1, Line 639: TotalEarlySenders",
> Should we have a separate counter for the early sender batches?
'total_early_senders_' above is for that.


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:

http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-sender.cc@635
PS1, Line 635:   profile()->AddDerivedCounter("NetworkThroughput", 
TUnit::BYTES_PER_SECOND,
 :   bind(::UnitsPerSecond, 
bytes_sent_counter_,
 :  

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9690/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9690/1//COMMIT_MSG@48
PS1, Line 48: DeferredBatches
> Should we rename it to DeferredBatchQueueSize or DeferredQueueSize or Defer
Done


http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc
File be/src/runtime/krpc-data-stream-recvr.cc:

http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc@639
PS1, Line 639: TotalBatchesDeferred
> But that is the number of hosts arriving early, not the number of batches a
Since we let one pending RPC per sender to a given exchange node instance at a 
time, this should be the same as the number of "hosts" if you only count the 
fragment instances sending to this exchange node.

Of course, there could be multiple queries running on the remote host so 
different fragment instances for the same or different queries could arrive 
early but not sure if they are too meaningful to track outside of query profile.

Please let me know if I misunderstood your concern.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Thu, 22 Mar 2018 00:34:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Hello Lars Volker, Mostafa Mokhtar, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9690

to look at the new patch set (#3).

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..

IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

This change implements a couple of improvements to the profiles of
KrpcDataStreamRecvr and KrpcDataStreamSender:

- track pending number of deferred row batches over time in KrpcDataStreamRecvr
- track the number of bytes dequeued over time in KrpcDataStreamRecvr
- track the min/max/avg of time row batches spent in deferred queue
- track the number of bytes sent from KrpcDataStreamSender over time

Sample profile of an Exchange node instance:

  EXCHANGE_NODE (id=22):(Total: 378.797ms, non-child: 123.042ms, % 
non-child: 32.48%)
 - ConvertRowBatchTime: 70.316ms
 - PeakMemoryUsage: 80.93 KB (82875)
 - RowsReturned: 66.86K (66855)
 - RowsReturnedRate: 176.49 K/sec
Buffer pool:
   - AllocTime: 271.165us
   - CumulativeAllocationBytes: 1.54 MB (1613824)
   - CumulativeAllocations: 132 (132)
   - PeakReservation: 80.00 KB (81920)
   - PeakUnpinnedBytes: 0
   - PeakUsedReservation: 80.00 KB (81920)
   - ReadIoBytes: 0
   - ReadIoOps: 0 (0)
   - ReadIoWaitTime: 0.000ns
   - WriteIoBytes: 0
   - WriteIoOps: 0 (0)
   - WriteIoWaitTime: 0.000ns
Dequeue:
  BytesDequeued(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1.34 MB
   - FirstBatchWaitTime: 0.000ns
   - TotalBytesDequeued: 1.34 MB (1403955)
   - TotalEosReceived: 1 (1)
   - TotalGetBatchTime: 308.074ms
 - DataWaitTime: 255.763ms
Enqueue:
  BytesReceived(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 33.17 KB, 33.17 
KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 
KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 
KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 KB, 33.17 
KB, 33.17 KB, 1.06 MB
  DeferredQueueSize(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0
   - DeferredQueueTime: (Avg: 324.328ms ; Min: 316.144us ; Max: 
13s611ms ; Number of samples: 43)
   - DeserializeRowBatchTime: 1.764ms
   - TotalBatchesDeferred: 43 (43)
   - TotalBatchesEnqueued: 66 (66)
   - TotalBatchesReceived: 66 (66)
   - TotalBytesReceived: 1.06 MB (1114795)
   - TotalEarlySenders: 0 (0)

Sample sender's profile:

KrpcDataStreamSender (dst_id=22):(Total: 83.234ms, non-child: 83.234ms, 
% non-child: 100.00%)
  BytesSent(500.000ms): 0, 0, 0, 0, 0, 0, 0, 0, 0, 16.60 KB, 16.60 KB, 
16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 
16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 
16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 16.60 KB, 
16.60 KB
   - EosSent: 1 (1)
   - NetworkThroughput: 21.08 MB/sec
   - PeakMemoryUsage: 9.84 KB (10080)
   - RowsReturned: 66.86K (66855)
   - RpcFailure: 0 (0)
   - RpcRetry: 0 (0)
   - SerializeBatchTime: 24.319ms
   - TotalBytesSent: 1.06 MB (1114795)
   - TotalNetworkTime: 50.424ms
   - UncompressedRowBatchSize: 1.34 MB (1403955)

Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/runtime-state.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M common/protobuf/data_stream_service.proto
11 files changed, 227 insertions(+), 129 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9690/3
--
To view, visit http://gerrit.cloudera.org:8080/9690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9748 )

Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I469feff940fdd07e1e407c9df49de79ed303151e
Gerrit-Change-Number: 9748
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 22 Mar 2018 00:43:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6694: fix "buffer pool" child profile order

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9749 )

Change subject: IMPALA-6694: fix "buffer pool" child profile order
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9749/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/9749/1/be/src/util/runtime-profile.cc@359
PS1, Line 359: while (insert_pos != children_.end() && !found_child) {
 :   found_child = insert_pos->first == child;
 :   ++insert_pos;
 : }
Is it fair to expect that children_ here should end up having the same number 
of entries as nodes[] after this loop is over ?

If so, for entries in nodes[] which don't already exist in child_map_, should 
we expect the insertion position be "i" in this case ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230f0673edf20a846fdb13191b7a292d329c1bb8
Gerrit-Change-Number: 9749
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 23 Mar 2018 23:47:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9690 )

Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and 
KrpcDataStreamSender
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h
File be/src/runtime/krpc-data-stream-recvr.h:

http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h@263
PS1, Line 263:   /// Total wall-clock time of row batches spent in deferred 
queue before being processed.
 :   RuntimeProfile::Counter* deferred_queue_time_;
Instead of tracking the cumulative time of RPCs spent in the deferred queue, 
tracking the min/max/avg of the time spent in deferred queue is more intuitive 
and useful.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd
Gerrit-Change-Number: 9690
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Tue, 20 Mar 2018 01:49:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6554: Fix a race in DequeueDeferredRpc()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9446 )

Change subject: IMPALA-6554: Fix a race in DequeueDeferredRpc()
..


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9446/4/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9446/4/be/src/runtime/krpc-data-stream-mgr.cc@231
PS4, Line 231: AndRelea
> in this case, the "deferred" doesn't make sense since it hasn't yet been di
Agreed. Fixed in PS5.


http://gerrit.cloudera.org:8080/#/c/9446/4/be/src/runtime/krpc-data-stream-mgr.cc@287
PS4, Line 287: AndRelea
> this case also not deferred
Done


http://gerrit.cloudera.org:8080/#/c/9446/4/be/src/runtime/krpc-data-stream-recvr.h
File be/src/runtime/krpc-data-stream-recvr.h:

http://gerrit.cloudera.org:8080/#/c/9446/4/be/src/runtime/krpc-data-stream-recvr.h@175
PS4, Line 175: deferred_rpc
> I think it'd be clearer to also rename this field to deferred_rpc_tracker_
Done


http://gerrit.cloudera.org:8080/#/c/9446/4/be/src/service/data-stream-service.h
File be/src/service/data-stream-service.h:

http://gerrit.cloudera.org:8080/#/c/9446/4/be/src/service/data-stream-service.h@62
PS4, Line 62: AndRelea
> as mentioned earlier, since this is used for non-deferred case as well, let
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a3b0789633c7b8bc898381d509e2af769f0e069
Gerrit-Change-Number: 9446
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 01 Mar 2018 21:53:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6554: Fix a race in DequeueDeferredRpc()

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9446

to look at the new patch set (#5).

Change subject: IMPALA-6554: Fix a race in DequeueDeferredRpc()
..

IMPALA-6554: Fix a race in DequeueDeferredRpc()

Previously, KrpcDataStreamRecvr::DequeueDeferredRpc() will call
RespondAndReleaseRpc() outside the of sender queue's lock. Another
thread can race ahead and call KrpcDataStreamRecvr::Close() before
MemTracker::Release() is called on the receiver's MemTracker. This
may lead to update to the receiver's MemTracker after it has been
closed. This change fixes the problem by updating the MemTracker
before dropping the lock in DequeueDeferredRpc(). This change also
changes RespondAndReleaseRpc() to update the MemTracker first before
responding to the RPC. The order doesn't really matter much because
the response of an RPC is handled asynchronously by reactor threads so
there is always a window between when the MemTracker is updated and when
the actual RPC payload is freed. The order is updated so it's consistent
with that of DequeueDeferredRpc().

Change-Id: I4a3b0789633c7b8bc898381d509e2af769f0e069
---
M be/src/rpc/impala-service-pool.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
7 files changed, 60 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/9446/5
--
To view, visit http://gerrit.cloudera.org:8080/9446
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a3b0789633c7b8bc898381d509e2af769f0e069
Gerrit-Change-Number: 9446
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-2567: Enable KRPC by default

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9461 )

Change subject: IMPALA-2567: Enable KRPC by default
..


Patch Set 6: Code-Review+2

Rebased and reverted bumping the memory limit in test_row_filters() as that's 
not necessary anymore.

Carry Dan's +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
Gerrit-Change-Number: 9461
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 05 Mar 2018 05:10:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2567: Enable KRPC by default

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9461

to look at the new patch set (#5).

Change subject: IMPALA-2567: Enable KRPC by default
..

IMPALA-2567: Enable KRPC by default

This change enables the switch to use KRPC by default.
This change also fixes a bug in KrpcDataStreamMgr to
check if maintenance thread was started before calling
Join() on it. This shows up in BE tests as the maintenance
thread isn't started in them.

Also bumped the memory limit for a query in test_row_filters().
It seems to fail consistently in exhastive build after recent
scanner memory changes.

Testing done: exhaustive build.

Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M bin/run-all-tests.sh
M bin/start-impala-cluster.py
M tests/common/custom_cluster_test_suite.py
M tests/common/skip.py
M tests/common/test_skip.py
M tests/conftest.py
8 files changed, 25 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/9461/5
--
To view, visit http://gerrit.cloudera.org:8080/9461
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
Gerrit-Change-Number: 9461
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-2567: Enable KRPC by default

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9461

to look at the new patch set (#6).

Change subject: IMPALA-2567: Enable KRPC by default
..

IMPALA-2567: Enable KRPC by default

This change enables the switch to use KRPC by default.
This change also fixes a bug in KrpcDataStreamMgr to
check if maintenance thread was started before calling
Join() on it. This shows up in BE tests as the maintenance
thread isn't started in them.

Testing done: exhaustive build.

Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M bin/run-all-tests.sh
M bin/start-impala-cluster.py
M tests/common/custom_cluster_test_suite.py
M tests/common/skip.py
M tests/common/test_skip.py
M tests/conftest.py
8 files changed, 25 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/9461/6
--
To view, visit http://gerrit.cloudera.org:8080/9461
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
Gerrit-Change-Number: 9461
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-2567: Enable KRPC by default

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9461

to look at the new patch set (#3).

Change subject: IMPALA-2567: Enable KRPC by default
..

IMPALA-2567: Enable KRPC by default

This change enables the switch to use KRPC by default.
This change also fixes a bug in KrpcDataStreamMgr to
check if maintenance thread was started before calling
Join() on it. This shows up in BE tests as the maintenance
thread isn't started in them.

Also bumped the memory limit for a query in test_row_filters().
It seems to fail consistently in exhastive build after recent
scanner memory changes.

Testing done: exhaustive build.

Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M bin/run-all-tests.sh
M bin/start-impala-cluster.py
M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test
M tests/common/custom_cluster_test_suite.py
M tests/common/skip.py
M tests/common/test_skip.py
M tests/conftest.py
9 files changed, 26 insertions(+), 27 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/9461/3
--
To view, visit http://gerrit.cloudera.org:8080/9461
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
Gerrit-Change-Number: 9461
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-2567: Enable KRPC by default

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9461 )

Change subject: IMPALA-2567: Enable KRPC by default
..


Patch Set 2:

(3 comments)

The existing jenkins jobs to test with KRPC enabled will be replaced with tests 
with Thrift enabled.

http://gerrit.cloudera.org:8080/#/c/9461/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/9461/2/be/src/runtime/exec-env.cc@83
PS2, Line 83: Used to indicate whether to use KRPC for the "
: "DataStream subsystem, or the Thrift RPC layer instead.
> This could be clearer:
Done


http://gerrit.cloudera.org:8080/#/c/9461/2/bin/run-all-tests.sh
File bin/run-all-tests.sh:

http://gerrit.cloudera.org:8080/#/c/9461/2/bin/run-all-tests.sh@70
PS2, Line 70: if [[ "${TEST_KRPC}" != "false" ]]; then
:   TEST_START_CLUSTER_ARGS="${TEST_START_CLUSTER_ARGS} --use_krpc"
: fi
> and this
Done


http://gerrit.cloudera.org:8080/#/c/9461/2/bin/run-all-tests.sh@124
PS2, Line 124: if [[ "${TEST_KRPC}" != "false" ]]; then
 :   COMMON_PYTEST_ARGS+=" --test_krpc"
 : fi
> I think this needs to be adjusted (so we can test with KRPC disabled)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
Gerrit-Change-Number: 9461
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 02 Mar 2018 18:59:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2567: Enable KRPC by default

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9461 )

Change subject: IMPALA-2567: Enable KRPC by default
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9461/3/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

http://gerrit.cloudera.org:8080/#/c/9461/3/bin/start-impala-cluster.py@57
PS3, Line 57: , help
> thrift is always enabled. How about "Enable Thrift DataStream service ..."
Done


http://gerrit.cloudera.org:8080/#/c/9461/3/tests/common/test_skip.py
File tests/common/test_skip.py:

http://gerrit.cloudera.org:8080/#/c/9461/3/tests/common/test_skip.py@35
PS3, Line 35: assert not pytest.config.option.test_no_krpc
:
:   @SkipIf.not_thrift
:   def test_skip_if_not_thrift(self):
: assert pytest.config.option.test_no_krpc
> these look backwards. don't you have to move the 'not'?
Oops. Fixed.


http://gerrit.cloudera.org:8080/#/c/9461/3/tests/conftest.py
File tests/conftest.py:

http://gerrit.cloudera.org:8080/#/c/9461/3/tests/conftest.py@120
PS3, Line 120: all tests with
> Thrift DataStream service enabled.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
Gerrit-Change-Number: 9461
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 02 Mar 2018 22:11:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2567: Enable KRPC by default

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9461

to look at the new patch set (#4).

Change subject: IMPALA-2567: Enable KRPC by default
..

IMPALA-2567: Enable KRPC by default

This change enables the switch to use KRPC by default.
This change also fixes a bug in KrpcDataStreamMgr to
check if maintenance thread was started before calling
Join() on it. This shows up in BE tests as the maintenance
thread isn't started in them.

Also bumped the memory limit for a query in test_row_filters().
It seems to fail consistently in exhastive build after recent
scanner memory changes.

Testing done: exhaustive build.

Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
---
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M bin/run-all-tests.sh
M bin/start-impala-cluster.py
M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test
M tests/common/custom_cluster_test_suite.py
M tests/common/skip.py
M tests/common/test_skip.py
M tests/conftest.py
9 files changed, 26 insertions(+), 27 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/9461/4
--
To view, visit http://gerrit.cloudera.org:8080/9461
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
Gerrit-Change-Number: 9461
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-2567: Enable KRPC by default

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9461 )

Change subject: IMPALA-2567: Enable KRPC by default
..


Patch Set 4:

Hold off from merging until the builds are in better shape.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae736c1c1351758969b4d84e34fc5b2d048660a0
Gerrit-Change-Number: 9461
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:04:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6347: Monitor queue depth size for outgoing RPCs for Reactor threads

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9384 )

Change subject: IMPALA-6347: Monitor queue depth size for outgoing RPCs for 
Reactor threads
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9384/11/www/rpcz.tmpl
File www/rpcz.tmpl:

http://gerrit.cloudera.org:8080/#/c/9384/11/www/rpcz.tmpl@108
PS11, Line 108: RPC
Thrift RPC ?


http://gerrit.cloudera.org:8080/#/c/9384/11/www/rpcz.tmpl@230
PS11, Line 230: $.map(json["per_conn_metrics"], function(row) {
  : return [$.map(row, function(cell) { return cell; })];
Does this rely on the order of the fields in the json document ? Seems a bit 
fragile when we add / remove or reorder fields in the future in rpc-mgr.cc. 
Will using the approach of update_krpc_services() be a bit more robust ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84fea531e98d3e84fcc57bf7533655218bc91f4c
Gerrit-Change-Number: 9384
Gerrit-PatchSet: 11
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Sat, 03 Mar 2018 02:18:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7163: Implement a state machine for the QueryState class

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10813 )

Change subject: IMPALA-7163: Implement a state machine for the QueryState class
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc@209
PS2, Line 209:   {BackendExecState::INITIALIZED, "INITIALIZED"},
 :   {BackendExecState::STARTED, "STARTED"},
 :   {BackendExecState::PREPARED, "PREPARED"},
 :   {BackendExecState::OPENED, "OPENED"},
 :   {BackendExecState::FINISHED, "FINISHED"},
 :   {BackendExecState::CANCELLED, "CANCELLED"},
 :   {BackendExecState::ERROR, "ERROR"}};
 :   return exec_state_to_str.at(state);
As discussed offline, it seems we can simplify this a bit by merging 
INITIALIZED and STARTED states and get rid of OPENED state altogether.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec5670a7db83ecae4656d7bb2ea372d3767ba7fe
Gerrit-Change-Number: 10813
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 28 Jun 2018 22:08:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#14). ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..

IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

This change converts ReportExecStatus() RPC from thrift
based RPC to KRPC. This is done in part of the preparation
for fixing IMPALA-2990 as we can take advantage of TCP connection
multiplexing in KRPC to avoid overwhelming the coordinator
with too many connections by reducing the number of TCP connection
to one for each executor.

This patch also introduces a new service pool for all query execution
control related RPCs in the future so that control commands from
coordinators aren't blocked by long-running DataStream services' RPCs.
The majority of this patch is mechanical conversion of some Thrift
structures used in ReportExecStatus() RPC to Protobuf. Note that the
runtime profile is still retained as a Thrift structure as Impala
clients will still fetch query profiles using Thrift RPCs. This also
avoids duplicating the serialization implementation in both Thrift
and Protobuf for the runtime profile. The Thrift runtime profiles
are serialized and sent as a sidecar in ReportExecStatus() RPC.

This patch also fixes IMPALA-7241 which may lead to duplicated
dml stats being applied. The fix is by adding a monotonically
increasing version number for fragment instances' reports. The
coordinator will ignore any report smaller than or equal to the
version in the last report.

Testing done:
1. Exhaustive build.
2. Added some targeted test cases for profile serialization failure and RPC 
retries/timeout.

Change-Id: I7638583b433dcac066b87198e448743d90415ebe
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/common/global-flags.cc
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-writer.cc
M be/src/exec/hdfs-table-writer.h
M be/src/rpc/CMakeLists.txt
M be/src/rpc/jni-thrift-util.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.h
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/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/dml-exec-state.cc
M be/src/runtime/dml-exec-state.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/service/CMakeLists.txt
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
A be/src/service/control-service.cc
A be/src/service/control-service.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
A be/src/util/error-util-internal.h
M be/src/util/error-util-test.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
M be/src/util/runtime-profile.cc
M be/src/util/uid-util.h
M common/protobuf/CMakeLists.txt
M common/protobuf/common.proto
A common/protobuf/control_service.proto
M common/protobuf/data_stream_service.proto
M common/protobuf/row_batch.proto
M common/protobuf/rpc_test.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_rpc_timeout.py
60 files changed, 1,211 insertions(+), 764 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10855/14
--
To view, visit http://gerrit.cloudera.org:8080/10855
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..


Patch Set 14:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:

http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.h@152
PS13, Line 152: /// Updates 'this' with exec_status and the fragment intance's 
thrift prof
> comment is out of date
Done


http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.cc@523
PS13, Line 523:   if (rows_counter != nullptr) {
> line too long (91 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.cc@530
PS13, Line 530: }
> line too long (91 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/service/control-service.cc
File be/src/service/control-service.cc:

http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/service/control-service.cc@42
PS13, Line 42: QUEUE_LIMIT_MSG
> Looks like a negative value here gives no limit? Maybe worth mentioning.
Thanks for pointing that out. The handling of negative value is fixed in new PS.

ParseUtil::ParseMemSpec() already distinguishes the input value between 
absolute value and percentage. If the input is percentage, it will compute the 
memory limit based on the reference value passed to ParseMemSpec().


http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/service/control-service.cc@46
PS13, Line 46: if left at default value 0
> or negative
Wording fixed.


http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:

http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@50
PS13, Line 50:   optional KuduDmlStatsPB kudu_stats = 3;
> is this, and elsewhere, referring to anything other than renaming, eg. to D
Looking at 
(https://gerrit.cloudera.org/#/c/4985/1/common/thrift/ImpalaInternalService.thrift@430),
 it seems to suggest more than just renaming the struct. It also involves some 
sort of refactoring.

That said, if we don't really know the meaning of this TODO statement, we can 
drop it too. What's your take ?


http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@107
PS13, Line 107:
  :   FIRST_BATCH_PRODUCED
> is this true if an error is encountered?
Copied and pasted from Thrift Implementation. Fixed.


http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@154
PS13, Line 154: individual fra
> I guess this and elsewhere was copied from the original thrift definition.
I am not 100% sure about the historical context but I believe the rationale 
behind all the RPC versioning is that we can have backward compatibility 
between different versions of Impala (See also 
https://gerrit.cloudera.org/#/c/6535/4/common/thrift/ImpalaInternalService.thrift@698)

That said, I don't think we ever implemented any true support for mixed 
versions of Impala (which is very useful for rolling upgrade). May be it's fair 
to drop the version string for now until we commit to supporting mixed version 
of Impala. In which case, we can take whatever snapshot of the structs then as 
V1. Before that, having the version string seems like some cruft that we don't 
need at the moment.

Also, I don't quite understand the rationale behind the required fields in some 
of the existing Thrift structs as that seems to make it hard to change the 
definition of the struct later. So, for now, I am marking all fields to be 
optional (and latter version of protobuf actually gets rid of required/optional 
so everything is optional anyway).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 08 Oct 2018 06:14:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11425 )

Change subject: IMPALA-2063 Remove newline characters in query status.
..


Patch Set 11: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 11
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Comment-Date: Fri, 05 Oct 2018 18:33:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11425 )

Change subject: IMPALA-2063 Remove newline characters in query status.
..


Patch Set 12: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 12
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Comment-Date: Fri, 05 Oct 2018 18:34:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2063 Remove newline characters in query status.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11425 )

Change subject: IMPALA-2063 Remove newline characters in query status.
..


Patch Set 10: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11425/10/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/11425/10/be/src/util/runtime-profile.cc@41
PS10, Line 41:
nit: blank line



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bbd7d7fe2c6d0f3799d0e6b336710bccfef0ab1
Gerrit-Change-Number: 11425
Gerrit-PatchSet: 10
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Comment-Date: Fri, 05 Oct 2018 18:24:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11615


Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..

IMPALA-4063: Merge report of query fragment instances per executor

Previously, each fragment instance executing on an executor will
independently report its status to the coordinator periodically.
This creates a huge amount of RPCs to the coordinator under highly
conurrent workloads, causing lock contention in the coordinator's
backend states as each fragment instance independently tries to
update the coordinator backend states. In addition, due to the lack
of coordinator between query fragment instances, a query may end
without collecting the profiles from all fragment instances when
one of them hits an error before some other fragment instance manages
to finish calling Prepare(), leading to missing profile for certain
fragment instances.

This change fixes the problem above by making a thread per QueryState
(started by QueryExecMgr) to be responsible for periodically reporting
the status and profiles of all fragment instances of a query running
on a backend. As part of this refactoring, each query fragment instance
will not report their errors individually. Instead, there is a cumulative
status maintained per QueryState. It's set to the error status of the first
fragment instance which hits an error or any general error (e.g. failure
to start a thread). With this change, the status reporting thread is
also removed.

Testing done: exhaustive tests

This patch is based on a patch by Sailesh Mukil

Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/control-service.cc
M common/protobuf/control_service.proto
M common/thrift/RuntimeProfile.thrift
M 
testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
16 files changed, 377 insertions(+), 420 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/11615/1
--
To view, visit http://gerrit.cloudera.org:8080/11615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[Impala-ASF-CR] IMPALA-7704: Revert "IMPALA-7644: Hide Parquet page index writing with feature flag"

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11671 )

Change subject: IMPALA-7704: Revert "IMPALA-7644: Hide Parquet page index 
writing with feature flag"
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf0a64d6ec747275e3ecd6e801e054f81095591a
Gerrit-Change-Number: 11671
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Sat, 13 Oct 2018 01:00:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11535/9/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

http://gerrit.cloudera.org:8080/#/c/11535/9/testdata/workloads/functional-query/queries/QueryTest/exprs.test@25
PS9, Line 25: group by Z
> The new test case still seems to be group-by a single column. May be you ca
This is not yet addressed


http://gerrit.cloudera.org:8080/#/c/11535/11/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

http://gerrit.cloudera.org:8080/#/c/11535/11/testdata/workloads/functional-query/queries/QueryTest/exprs.test@12
PS11, Line 12:  GROUP BY of NaN values aggregates NaN's as one grouping
 : select count(*), sqrt(0.5-x) as Z
 : from (VALUES((1.6 x, 2 y), (3.2, 4), (5.4,6))) T
 : group by Z
 :  RESULTS
 : 3, NaN
 :  TYPES
 : bigint, double
 : 
 :  QUERY
 : # GROUP BY of NaN values aggregates NaN's as one grouping
 : select count(*), cast(sqrt(0.5-x) as FLOAT) as Z
 : from (VALUES((1.6 x, 2 y), (3.2, 4), (5.4,6), (0.5, 2), (0.5, 
3), (-0.5, 1))) T
 : group by Z order by Z
 :  RESULTS
 : 3, NaN
 : 2, 0
 : 1, 1
 :  TYPES
 : bigint, float
 : 
 :  QUERY
> These tests are testing the behavior of group-by so they should belong to a
This comment is not yet addressed



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 11
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 13 Oct 2018 04:42:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 11:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@748
PS9, Line 748:   llvm::Value* cmp_raw = builder_->CreateFCmpOEQ(local_val, 
val, "cmp_raw");
 :   if (!inclusive_equality) return cmp_raw;
> I need to generate that comparison in either case, so if I followed you sug
Good point. Missed that it's used twice.


http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@801
PS9, Line 801:
> There is no "inclusive_equality" flag here.  The restriction to probe-only
The probe-only behavior here doesn't match the behavior of the non-codegen'd 
version of the code (i.e. EvalRow which is called by EvalBuildRow() and 
EvalProbeRow()) which unconditionally converts Nan to its canonical form.


http://gerrit.cloudera.org:8080/#/c/11535/11/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

http://gerrit.cloudera.org:8080/#/c/11535/11/be/src/exec/hash-table.cc@237
PS11, Line 237: INCLUSIVE_EQUALITY
This change in the logic of hash table may have implication to Join too.

It may warrant a test case to verify the behavior of Nan <=> Nan (which should 
evaluate to false isn't changed after this patch in this file: 
https://github.com/apache/impala/blob/master/testdata/workloads/functional-query/queries/QueryTest/joins.test

Please see the section "Handling NULLs in Join Columns" here 
(https://www.cloudera.com/documentation/enterprise/latest/topics/impala_joins.html)
 for background.

Can you please also verify the behavior of join on columns with Nan value 
doesn't change after this patch ?

  create table nan_test (c1 float, c2 float);

  insert into nan_test select cast(sqrt(0.5-x) as FLOAT), cast(sqrt(3.5-y) as 
FLOAT) from (VALUES((1.6 x, 2 y), (3.2, 4), (5.4,6), (0.5, 2), (0.5, 3), (-0.5, 
1))) T;

  select t1.c1,t2.c1 from nan_test t1 right outer join nan_test t2 on t1.c1 = 
t2.c1;


http://gerrit.cloudera.org:8080/#/c/11535/11/be/src/exec/hash-table.cc@256
PS11, Line 256: val_is_ambiguous
nit: val_is_nan


http://gerrit.cloudera.org:8080/#/c/11535/11/be/src/exec/hash-table.cc@257
PS11, Line 257: local_is_ambiguous
nit: local_is_nan


http://gerrit.cloudera.org:8080/#/c/11535/11/be/src/runtime/raw-value.h
File be/src/runtime/raw-value.h:

http://gerrit.cloudera.org:8080/#/c/11535/11/be/src/runtime/raw-value.h@40
PS11, Line 40:
nit: extra space


http://gerrit.cloudera.org:8080/#/c/11535/9/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

http://gerrit.cloudera.org:8080/#/c/11535/9/testdata/workloads/functional-query/queries/QueryTest/exprs.test@25
PS9, Line 25: group by Z
> Done
The new test case still seems to be group-by a single column. May be you can 
try something like:

  select count(*), cast(sqrt(0.5-x) as FLOAT) as Z, cast(sqrt(0.5+x) as FLOAT) 
as Y
   from (VALUES((1.6 x, 2 y), (3.2, 4), (5.4,6), (0.5, 2), (0.5, 3), (-0.5, 
1))) T
   group by Y,Z;


http://gerrit.cloudera.org:8080/#/c/11535/11/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

http://gerrit.cloudera.org:8080/#/c/11535/11/testdata/workloads/functional-query/queries/QueryTest/exprs.test@12
PS11, Line 12:  GROUP BY of NaN values aggregates NaN's as one grouping
 : select count(*), sqrt(0.5-x) as Z
 : from (VALUES((1.6 x, 2 y), (3.2, 4), (5.4,6))) T
 : group by Z
 :  RESULTS
 : 3, NaN
 :  TYPES
 : bigint, double
 : 
 :  QUERY
 : # GROUP BY of NaN values aggregates NaN's as one grouping
 : select count(*), cast(sqrt(0.5-x) as FLOAT) as Z
 : from (VALUES((1.6 x, 2 y), (3.2, 4), (5.4,6), (0.5, 2), (0.5, 
3), (-0.5, 1))) T
 : group by Z order by Z
 :  RESULTS
 : 3, NaN
 : 2, 0
 : 1, 1
 :  TYPES
 : bigint, float
 : 
 :  QUERY
These tests are testing the behavior of group-by so they should belong to 
aggregation.test



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 11
Gerrit-Owner: Michal Ostrowski 

[Impala-ASF-CR] IMPALA-7704: Revert "IMPALA-7644: Hide Parquet page index writing with feature flag"

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11671 )

Change subject: IMPALA-7704: Revert "IMPALA-7644: Hide Parquet page index 
writing with feature flag"
..


Patch Set 1:

Is this a clean revert ?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf0a64d6ec747275e3ecd6e801e054f81095591a
Gerrit-Change-Number: 11671
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Sat, 13 Oct 2018 00:40:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#17). ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..

IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

This change converts ReportExecStatus() RPC from thrift
based RPC to KRPC. This is done in part of the preparation
for fixing IMPALA-2990 as we can take advantage of TCP connection
multiplexing in KRPC to avoid overwhelming the coordinator
with too many connections by reducing the number of TCP connection
to one for each executor.

This patch also introduces a new service pool for all query execution
control related RPCs in the future so that control commands from
coordinators aren't blocked by long-running DataStream services' RPCs.
To avoid unnecessary delays due to sharing the network connections
between DataStream service and Control service, this change added the
service name as part of the user credentials for the ConnectionId
so each service will use a separate connection.

The majority of this patch is mechanical conversion of some Thrift
structures used in ReportExecStatus() RPC to Protobuf. Note that the
runtime profile is still retained as a Thrift structure as Impala
clients will still fetch query profiles using Thrift RPCs. This also
avoids duplicating the serialization implementation in both Thrift
and Protobuf for the runtime profile. The Thrift runtime profiles
are serialized and sent as a sidecar in ReportExecStatus() RPC.

This patch also fixes IMPALA-7241 which may lead to duplicated
dml stats being applied. The fix is by adding a monotonically
increasing version number for fragment instances' reports. The
coordinator will ignore any report smaller than or equal to the
version in the last report.

Testing done:
1. Exhaustive build.
2. Added some targeted test cases for profile serialization failure and RPC 
retries/timeout.

Change-Id: I7638583b433dcac066b87198e448743d90415ebe
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/common/global-flags.cc
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-writer.cc
M be/src/exec/hdfs-table-writer.h
M be/src/rpc/CMakeLists.txt
M be/src/rpc/jni-thrift-util.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-mgr.inline.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.h
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/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/dml-exec-state.cc
M be/src/runtime/dml-exec-state.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/service/CMakeLists.txt
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
A be/src/service/control-service.cc
A be/src/service/control-service.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
A be/src/util/error-util-internal.h
M be/src/util/error-util-test.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
M be/src/util/runtime-profile.cc
M be/src/util/uid-util.h
M common/protobuf/CMakeLists.txt
M common/protobuf/common.proto
A common/protobuf/control_service.proto
M common/protobuf/data_stream_service.proto
M common/protobuf/row_batch.proto
M common/protobuf/rpc_test.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_rpc_timeout.py
66 files changed, 1,269 insertions(+), 771 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10855/17
--
To view, visit http://gerrit.cloudera.org:8080/10855
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 17
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..


Patch Set 16:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/rpc/rpc-mgr.h@146
PS16, Line 146: service_name
> admittedly it's clever that you were able to hack the username in order to
I didn't realize the existence of static_service_name(). Thanks for pointing 
that out. I added a new template parameter S for this as the 
static_service_name or service_name doesn't seem available from the proxy.

I kind of implemented your suggestion for enum in this patch by defining 
GetProxy() in DataStreamService and ControlService respectively.


http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/coordinator-backend-state.cc@506
PS16, Line 506:   {
> why is this extra indentation block here? doesn't seem like there is any RA
Fixed. Forgot to revert the change after undoing the move for 
exec_summary_->lock


http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:

http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/fragment-instance-state.h@107
PS16, Line 107: const
> nit: const non-reference parameters dont make much sense
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 12 Oct 2018 06:12:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11535/14/testdata/workloads/functional-query/queries/QueryTest/joins.test
File testdata/workloads/functional-query/queries/QueryTest/joins.test:

http://gerrit.cloudera.org:8080/#/c/11535/14/testdata/workloads/functional-query/queries/QueryTest/joins.test@798
PS14, Line 798:  QUERY
Still missing a test case for nan <=> nan being false. Please see earlier 
comments.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 14
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 15 Oct 2018 18:35:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11535/14/testdata/workloads/functional-query/queries/QueryTest/joins.test
File testdata/workloads/functional-query/queries/QueryTest/joins.test:

http://gerrit.cloudera.org:8080/#/c/11535/14/testdata/workloads/functional-query/queries/QueryTest/joins.test@798
PS14, Line 798:  QUERY
> This test should cover that...  it compares NaN values and considers them t
Yes I think t1.c1 = t2.c1 is not the same as t1.c1 <=> t2.c1; Since your patch 
touches some logic in the hash table which indirectly affects <=> comparison 
case, it may be worth double checking nothing unexpected happens.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 14
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 15 Oct 2018 21:13:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Hello Thomas Marshall, Tim Armstrong, Joe McDonnell, Bikramjeet Vig, Impala 
Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/11615

to look at the new patch set (#5).

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..

IMPALA-4063: Merge report of query fragment instances per executor

Previously, each fragment instance executing on an executor will
independently report its status to the coordinator periodically.
This creates a huge amount of RPCs to the coordinator under highly
concurrent workloads, causing lock contention in the coordinator's
backend states when multiple fragment instances send them at the
same time. In addition, due to the lack of coordination between query
fragment instances, a query may end without collecting the profiles
from all fragment instances when one of them hits an error before
another fragment instance manages to finish Prepare(), leading to
missing profiles for certain fragment instances.

This change fixes the problem above by making a thread per QueryState
(started by QueryExecMgr) to be responsible for periodically reporting
the status and profiles of all fragment instances of a query running
on a backend. As part of this refactoring, each query fragment instance
will not report their errors individually. Instead, there is a cumulative
status maintained per QueryState. It's set to the error status of the first
fragment instance which hits an error or any general error (e.g. failure
to start a thread) when starting fragment instances. With this change,
the status reporting threads are also removed.

Testing done: exhaustive tests

This patch is based on a patch by Sailesh Mukil

Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M common/protobuf/control_service.proto
M common/thrift/RuntimeProfile.thrift
M 
testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
18 files changed, 389 insertions(+), 436 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/11615/5
--
To view, visit http://gerrit.cloudera.org:8080/11615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11615 )

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/fragment-instance-state.cc@444
PS3, Line 444:   if (fragment_ctx_.fragment.output_sink.type != 
TDataSinkType::PLAN_ROOT_SINK) {
 : // if we haven't already release this thread token in 
Prepare(), release it now
 : ReleaseThreadToken();
 :   }
> this is called at only one site and now does only one thing, should we just
This still seems like a conceptual step in the fragment life cycle so it makes 
sense to encapsulate it as a function.


http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:

http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-exec-mgr.h@78
PS3, Line 78: an instances hit
:   /// an error or
> nit: unless an instance hits an error they are cancelled
Done


http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-state.cc@397
PS3, Line 397: void QueryState::ErrorDuringExecute(const Status& status, const 
TUniqueId& finst_id) {
> previous patch had a racy check on the status, was there any benefit that e
It's okay to skip the racy call for now until evidence suggests otherwise. It's 
not a big deal if multiple fragment threads have to block when setting the 
error status. We don't expect this to be a common case so it's better to keep 
the code simpler and less error prone.


http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-state.cc@509
PS3, Line 509:   }
> nit: retain this comment here from previously at L506:
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:47:50 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7351: Add estimates to Exchange node

[Michael Ho (Code Review)]

Michael Ho has removed Michael Ho from this change.  ( 
http://gerrit.cloudera.org:8080/11692 )

Change subject: IMPALA-7351: Add estimates to Exchange node
..


Removed reviewer Michael Ho.
--
To view, visit http://gerrit.cloudera.org:8080/11692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I5b577f9511abc48b992e814d50bba4959f23f7fd
Gerrit-Change-Number: 11692
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] Revert "IMPALA-6910/IMPALA-7070: Increase log level for HDFS S3 code"

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11699 )

Change subject: Revert "IMPALA-6910/IMPALA-7070: Increase log level for HDFS S3 
code"
..


Patch Set 2:

Lars, what's the status of this patch ? Should it be abandoned ?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ab7f2f6317f1281928af5ea6cf3fd7d0c6e0a09
Gerrit-Change-Number: 11699
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 16 Oct 2018 23:41:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11650 )

Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11650/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11650/2//COMMIT_MSG@26
PS2, Line 26: I have been unable to repro the actual crash despite trying a 
large
:   variety of different things. However, with additional logging 
added
:   its clear that the MemPool is being used concurrently, which is
:   incorrect.
Can you reproduce the problem now with DFAKE_MUTEX ?


http://gerrit.cloudera.org:8080/#/c/11650/2/be/CMakeLists.txt
File be/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/11650/2/be/CMakeLists.txt@234
PS2, Line 234:  SET(CLANG_IR_CXX_FLAGS "${CLANG_IR_CXX_FLAGS}" "-DNDEBUG")
I think this makes sense. I wonder what the historical context for not doing it 
in the first place. I checked the code on some older releases from more than 4 
years ago and it was there all along. May be the codegen time was too slow back 
then to include all the debug code ?

How much slow down (if any) did you notice in debug builds with this change ?


http://gerrit.cloudera.org:8080/#/c/11650/2/be/src/runtime/mem-pool.h
File be/src/runtime/mem-pool.h:

http://gerrit.cloudera.org:8080/#/c/11650/2/be/src/runtime/mem-pool.h@135
PS2, Line 135: DFAKE_SCOPED_LOCK(mutex_);
nit: Feel free to ignore but it seems easier to just add DFAKE_SCOPED_LOCK() 
directly in Allocate(). That said, the current code also looks fine as-is.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29
Gerrit-Change-Number: 11650
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Comment-Date: Tue, 16 Oct 2018 23:40:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 15:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11535/15/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

http://gerrit.cloudera.org:8080/#/c/11535/15/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3075
PS15, Line 3075: # IMPALA-6661 - NaN should not evaluate as the same as any 
other NaN via <=>
   : WITH W AS (SELECT T.*, CAST(SQRT(X) AS FLOAT)  P, CAST(SQRT(Y) 
AS FLOAT) Q
   : FROM (VALUES((-1.0 X, -1.0 Y), (-1.0, 0), (0, -1.0), (0, 0))) 
T )
   : SELECT * FROM W WHERE W.Q<=>W.P
   :  RESULTS
   : 0,0,0,0
   :  TYPES
   : FLOAT, FLOAT, FLOAT, FLOAT
> This test case is here in response to your comment: "It may warrant a test
Sorry, I wasn't clear in my comment. I meant to say a test case which exercise 
the join condition on Nan <=> Nan out of caution. hash-table.cc logic is used 
both by join and aggregation.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 15
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 17 Oct 2018 01:51:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 15:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11535/15/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

http://gerrit.cloudera.org:8080/#/c/11535/15/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3075
PS15, Line 3075: # IMPALA-6661 - NaN should not evaluate as the same as any 
other NaN via <=>
   : WITH W AS (SELECT T.*, CAST(SQRT(X) AS FLOAT)  P, CAST(SQRT(Y) 
AS FLOAT) Q
   : FROM (VALUES((-1.0 X, -1.0 Y), (-1.0, 0), (0, -1.0), (0, 0))) 
T )
   : SELECT * FROM W WHERE W.Q<=>W.P
   :  RESULTS
   : 0,0,0,0
   :  TYPES
   : FLOAT, FLOAT, FLOAT, FLOAT
This is not the right test to exercise join code. You can check the query plan 
but it doesn't have a join in it. You can check the query plan by doing explain 
of the query in Impala shell and see if it matches your expectation. You can do 
a self-join like the following. Also, this test belongs to joins.test.

  with y as (select t1.float_col as v
   from functional.alltypessmall t1,
functional.alltypessmall t2
   where sqrt(3.5-t1.float_col) <=> sqrt(3.5-t2.float_col))
  select count(*) from y where is_nan(v);



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 15
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 17 Oct 2018 00:21:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11615 )

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11615/5/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/11615/5/be/src/runtime/query-state.cc@495
PS5, Line 495: error:
 :   // This point is reached if there were general errors to start 
query fragment instances.
 :   // Wait for all running fragment instances to finish preparing 
and report status to the
 :   // coordinator to start query cancellation.
> Since we are changing some error handling for thread creation, a way to tes
Thanks for pointing that out. We already have a test to exercise the thread 
creation failure here 
(https://github.com/apache/impala/blob/master/tests/failure/test_failpoints.py#L159-L173)
 but I think it's also a good idea to exercise the paths stress option you 
mentioned. Thanks for the suggestion.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 17 Oct 2018 00:31:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7351: Add estimates to Exchange node

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11692 )

Change subject: IMPALA-7351: Add estimates to Exchange node
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11692/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java
File fe/src/main/java/org/apache/impala/planner/ExchangeNode.java:

http://gerrit.cloudera.org:8080/#/c/11692/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@192
PS2, Line 192: There is also a deferred rpc queue which queues at max
 : // one rpc payload (containing the rowbatch) per sender 
in-case the row
 : // batch queue hits the previously mentioned soft limit
In the long run, we should consider either of the followings:
- enable spilling in exchange node
- throttle the sender until space opens up on exchange node. May waste a bit of 
network bandwidth in some cases due to resend.

I suppose either of the above may eventually get the memory consumption of an 
exchange node under control.


http://gerrit.cloudera.org:8080/#/c/11692/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@214
PS2, Line 214: // TODO: will this be different for a broadcast exchange?
Yes, for broadcasting, each exchange node will get the aggregate of all 
senders' produced row batches. May be worth having that distinction here.

Also, does the following line make assumption that there is no skew in 
distribution during data shuffling ? May be worth documenting that.


http://gerrit.cloudera.org:8080/#/c/11692/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@223
PS2, Line 223: cardinality_
How is this different from getCardinality() ?


http://gerrit.cloudera.org:8080/#/c/11692/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java
File fe/src/main/java/org/apache/impala/planner/PlanNode.java:

http://gerrit.cloudera.org:8080/#/c/11692/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@74
PS2, Line 74:
nit: extra blank line



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b577f9511abc48b992e814d50bba4959f23f7fd
Gerrit-Change-Number: 11692
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 23 Oct 2018 00:56:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 16:

(3 comments)

Hey sorry for not getting back earlier. Bogged down again with some users' 
issues which I have to help with.

http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
File testdata/workloads/functional-query/queries/QueryTest/aggregation.test:

http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1384
PS16, Line 1384: (VALUES((1.6 x, 2 y), (3.2, 4), (5.4,6))) T
Please see comments in joins.test


http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/joins.test
File testdata/workloads/functional-query/queries/QueryTest/joins.test:

http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/joins.test@801
PS16, Line 801: (VALUES((1.6 x, 0 y), (3.2, 1), (5.4,2), (0.5, 3), (0.5, 4), 
(-0.5, 5))) XX),
The problem with this kind of query with few number VALUES() is that codegen 
will be disabled as the planner knows the number of rows will be small. I think 
it may make sense to have another test cases to scan some sizable table. This 
is also a good test case to keep as this exercises the interpretation path.

Of course, one can also set the query option DISABLE_CODEGEN_ROWS_THRESHOLD to 
a small value but it seems better to have a more realistic test query with 
scans in there instead of joining two union nodes of constants.

You can check the query profile to see codegen is enabled in the HASH JOIN node.


http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/joins.test@818
PS16, Line 818: with q as (VALUES((cast(1.0 as FLOAT) x), (2.0))),
  :  r as (select t1.x from q t1, q t2 where sqrt(1.0-t1.x) <=> 
sqrt(1.0-t2.x))
  :  select * from r
If you plan to keep this test case, this can be simplified as:
  with q as (VALUES((cast(1.0 as FLOAT) x), (2.0)))
  select t1.x from q t1, q t2 where sqrt(1.0-t1.x) <=> sqrt(1.0-t2.x)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 16
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 19 Oct 2018 18:18:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Hello Thomas Marshall, Todd Lipcon, Tim Armstrong, Bikramjeet Vig, Impala 
Public Jenkins, Michal Ostrowski,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10855

to look at the new patch set (#19).

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..

IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

This change converts ReportExecStatus() RPC from thrift
based RPC to KRPC. This is done in part of the preparation
for fixing IMPALA-2990 as we can take advantage of TCP connection
multiplexing in KRPC to avoid overwhelming the coordinator
with too many connections by reducing the number of TCP connection
to one for each executor.

This patch also introduces a new service pool for all query execution
control related RPCs in the future so that control commands from
coordinators aren't blocked by long-running DataStream services' RPCs.
To avoid unnecessary delays due to sharing the network connections
between DataStream service and Control service, this change added the
service name as part of the user credentials for the ConnectionId
so each service will use a separate connection.

The majority of this patch is mechanical conversion of some Thrift
structures used in ReportExecStatus() RPC to Protobuf. Note that the
runtime profile is still retained as a Thrift structure as Impala
clients will still fetch query profiles using Thrift RPCs. This also
avoids duplicating the serialization implementation in both Thrift
and Protobuf for the runtime profile. The Thrift runtime profiles
are serialized and sent as a sidecar in ReportExecStatus() RPC.

This patch also fixes IMPALA-7241 which may lead to duplicated
dml stats being applied. The fix is by adding a monotonically
increasing version number for fragment instances' reports. The
coordinator will ignore any report smaller than or equal to the
version in the last report.

Testing done:
1. Exhaustive build.
2. Added some targeted test cases for profile serialization failure
   and RPC retries/timeout.

Change-Id: I7638583b433dcac066b87198e448743d90415ebe
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/common/global-flags.cc
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-writer.cc
M be/src/exec/hdfs-table-writer.h
M be/src/rpc/CMakeLists.txt
M be/src/rpc/jni-thrift-util.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/rpc-mgr.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.h
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/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/dml-exec-state.cc
M be/src/runtime/dml-exec-state.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/service/CMakeLists.txt
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
A be/src/service/control-service.cc
A be/src/service/control-service.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
A be/src/util/error-util-internal.h
M be/src/util/error-util-test.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
M be/src/util/runtime-profile.cc
M be/src/util/uid-util.h
M common/protobuf/CMakeLists.txt
M common/protobuf/common.proto
A common/protobuf/control_service.proto
M common/protobuf/data_stream_service.proto
M common/protobuf/row_batch.proto
M common/protobuf/rpc_test.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_rpc_timeout.py
65 files changed, 1,298 insertions(+), 769 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10855/19
--
To view, visit http://gerrit.cloudera.org:8080/10855
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: 

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..


Patch Set 18:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10855/18/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/10855/18/be/src/runtime/query-state.cc@363
PS18, Line 363: ERROR
> I'd use DFATAL so that it would at least crash in debug builds. (DFATAL tur
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 20 Oct 2018 01:52:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..


Patch Set 19: Code-Review+1

Carry +1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 20 Oct 2018 01:52:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11650 )

Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29
Gerrit-Change-Number: 11650
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Comment-Date: Thu, 18 Oct 2018 05:49:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11650 )

Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter
..


Patch Set 3: Code-Review+2

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11650/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11650/3//COMMIT_MSG@29
PS3, Line 29:
:   MemPool::Allocate.
Can you please consider adding this query as a test as a regression test ? 
Although it may not be reproducible as standalone, it will still exercise the 
path in question. Since it's also timing dependent, it may be more readily 
reproducible in our regular testing environment where we run multiple queries 
concurrently.


http://gerrit.cloudera.org:8080/#/c/11650/3/be/src/util/min-max-filter-test.cc
File be/src/util/min-max-filter-test.cc:

http://gerrit.cloudera.org:8080/#/c/11650/3/be/src/util/min-max-filter-test.cc@355
PS3, Line 355:   EXPECT_EQ(TimestampValue::FromTColumnValue(tFilter2.max), t3);
Should we call Close() on the filters above too ?


http://gerrit.cloudera.org:8080/#/c/11650/3/be/src/util/min-max-filter.h
File be/src/util/min-max-filter.h:

http://gerrit.cloudera.org:8080/#/c/11650/3/be/src/util/min-max-filter.h@188
PS3, Line 188: MemPool mem_pool_;
As mentioned previously, please add a comment for this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29
Gerrit-Change-Number: 11650
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Comment-Date: Thu, 18 Oct 2018 02:34:46 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..


Patch Set 18:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/10855/17//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10855/17//COMMIT_MSG@40
PS17, Line 40: 2. Added some targeted test cases for profile serialization 
failure
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/10855/17/be/src/rpc/rpc-mgr.inline.h
File be/src/rpc/rpc-mgr.inline.h:

http://gerrit.cloudera.org:8080/#/c/10855/17/be/src/rpc/rpc-mgr.inline.h@35
PS17, Line 35: std::unique_ptr* proxy) {
> hrm, it still feels like we are going through a lot of gymnastics to avoid
OK. Reverted to sharing the connection for now in this patch. The kudu patch 
(https://gerrit.cloudera.org/#/c/11681/) is already out for review.


http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/rpc/thrift-util-test.cc
File be/src/rpc/thrift-util-test.cc:

http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/rpc/thrift-util-test.cc@58
PS14, Line 58:
> Should we add a test for SerializeToString as well?
Done


http://gerrit.cloudera.org:8080/#/c/10855/17/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/10855/17/be/src/runtime/query-state.cc@356
PS17, Line 356:
> should we handle errors like these and log it instead?
Yes, while Todd previously commented that this should be a CHECK(), I think 
it's better to not crash Impala on a non-fatal issue. Admittedly, the system is 
most likely in a bad state if this fails but not being able to send the profile 
shouldn't be fatal either.


http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/service/control-service.h
File be/src/service/control-service.h:

http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/service/control-service.h@52
PS16, Line 52: e*
> nit: with
Done


http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/testutil/in-process-servers.cc
File be/src/testutil/in-process-servers.cc:

http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/testutil/in-process-servers.cc@51
PS14, Line 51:  // Thrift server c
> can you document the reason here? it was not clear to me as to why only thi
Done


http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/util/runtime-profile.cc@251
PS14, Line 251: if (UNLIKELY(nodes.size()) == 0) return;
> does this only happen when thrift de-serialization fails?
Or serialization failure on the executor side.


http://gerrit.cloudera.org:8080/#/c/10855/14/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:

http://gerrit.cloudera.org:8080/#/c/10855/14/common/protobuf/control_service.proto@27
PS14, Line 27: message ParquetDmlStatsPB {
> nit: maybe retain the comment in thrift file
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sun, 14 Oct 2018 02:21:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..


Patch Set 18: Code-Review+1

Carry +1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sun, 14 Oct 2018 02:21:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11615 )

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@243
PS2, Line 243: backend_exec_state_ = cur_state == BackendExecState::PREPARING ?
 :   BackendExecState::EXECUTING : 
BackendExecState::FINISHED;
> Sorry, to be clearer - I was suggesting leaving the logic around 'overall_s
There is one already at line 523.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sun, 14 Oct 2018 02:21:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Hello Thomas Marshall, Tim Armstrong, Joe McDonnell, Bikramjeet Vig, Impala 
Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/11615

to look at the new patch set (#4).

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..

IMPALA-4063: Merge report of query fragment instances per executor

Previously, each fragment instance executing on an executor will
independently report its status to the coordinator periodically.
This creates a huge amount of RPCs to the coordinator under highly
concurrent workloads, causing lock contention in the coordinator's
backend states as each fragment instance tries to them at the same
time. In addition, due to the lack of coordination between query
fragment instances, a query may end without collecting the profiles
from all fragment instances when one of them hits an error before
another fragment instance manages to finish calling Prepare(), leading
to missing profiles for certain fragment instances.

This change fixes the problem above by making a thread per QueryState
(started by QueryExecMgr) to be responsible for periodically reporting
the status and profiles of all fragment instances of a query running
on a backend. As part of this refactoring, each query fragment instance
will not report their errors individually. Instead, there is a cumulative
status maintained per QueryState. It's set to the error status of the first
fragment instance which hits an error or any general error (e.g. failure
to start a thread) when starting fragment instances. With this change,
the status reporting threads are also removed.

Testing done: exhaustive tests

This patch is based on a patch by Sailesh Mukil

Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M common/protobuf/control_service.proto
M common/thrift/RuntimeProfile.thrift
M 
testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
18 files changed, 387 insertions(+), 436 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/11615/4
--
To view, visit http://gerrit.cloudera.org:8080/11615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Hello Thomas Marshall, Todd Lipcon, Tim Armstrong, Bikramjeet Vig, Impala 
Public Jenkins, Michal Ostrowski,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10855

to look at the new patch set (#18).

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..

IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

This change converts ReportExecStatus() RPC from thrift
based RPC to KRPC. This is done in part of the preparation
for fixing IMPALA-2990 as we can take advantage of TCP connection
multiplexing in KRPC to avoid overwhelming the coordinator
with too many connections by reducing the number of TCP connection
to one for each executor.

This patch also introduces a new service pool for all query execution
control related RPCs in the future so that control commands from
coordinators aren't blocked by long-running DataStream services' RPCs.
To avoid unnecessary delays due to sharing the network connections
between DataStream service and Control service, this change added the
service name as part of the user credentials for the ConnectionId
so each service will use a separate connection.

The majority of this patch is mechanical conversion of some Thrift
structures used in ReportExecStatus() RPC to Protobuf. Note that the
runtime profile is still retained as a Thrift structure as Impala
clients will still fetch query profiles using Thrift RPCs. This also
avoids duplicating the serialization implementation in both Thrift
and Protobuf for the runtime profile. The Thrift runtime profiles
are serialized and sent as a sidecar in ReportExecStatus() RPC.

This patch also fixes IMPALA-7241 which may lead to duplicated
dml stats being applied. The fix is by adding a monotonically
increasing version number for fragment instances' reports. The
coordinator will ignore any report smaller than or equal to the
version in the last report.

Testing done:
1. Exhaustive build.
2. Added some targeted test cases for profile serialization failure
   and RPC retries/timeout.

Change-Id: I7638583b433dcac066b87198e448743d90415ebe
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/common/global-flags.cc
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-writer.cc
M be/src/exec/hdfs-table-writer.h
M be/src/rpc/CMakeLists.txt
M be/src/rpc/jni-thrift-util.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/rpc-mgr.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.h
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/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/dml-exec-state.cc
M be/src/runtime/dml-exec-state.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/service/CMakeLists.txt
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
A be/src/service/control-service.cc
A be/src/service/control-service.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
A be/src/util/error-util-internal.h
M be/src/util/error-util-test.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
M be/src/util/runtime-profile.cc
M be/src/util/uid-util.h
M common/protobuf/CMakeLists.txt
M common/protobuf/common.proto
A common/protobuf/control_service.proto
M common/protobuf/data_stream_service.proto
M common/protobuf/row_batch.proto
M common/protobuf/rpc_test.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_rpc_timeout.py
65 files changed, 1,298 insertions(+), 769 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10855/18
--
To view, visit http://gerrit.cloudera.org:8080/10855
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: 

[Impala-ASF-CR](asf-site) Download 3.0.1

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11766 )

Change subject: Download 3.0.1
..


Patch Set 1: Code-Review+2

Thanks for doing it.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: I044f119788e5c228bf41ed241ae6d23e5eb00a25
Gerrit-Change-Number: 11766
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 24 Oct 2018 17:56:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11615 )

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..


Patch Set 6: Code-Review+1

Carry +1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 25 Oct 2018 02:18:00 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Hello Thomas Marshall, Todd Lipcon, Tim Armstrong, Bikramjeet Vig, Impala 
Public Jenkins, Michal Ostrowski,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10855

to look at the new patch set (#20).

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..

IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

This change converts ReportExecStatus() RPC from thrift
based RPC to KRPC. This is done in part of the preparation
for fixing IMPALA-2990 as we can take advantage of TCP connection
multiplexing in KRPC to avoid overwhelming the coordinator
with too many connections by reducing the number of TCP connection
to one for each executor.

This patch also introduces a new service pool for all query execution
control related RPCs in the future so that control commands from
coordinators aren't blocked by long-running DataStream services' RPCs.
To avoid unnecessary delays due to sharing the network connections
between DataStream service and Control service, this change added the
service name as part of the user credentials for the ConnectionId
so each service will use a separate connection.

The majority of this patch is mechanical conversion of some Thrift
structures used in ReportExecStatus() RPC to Protobuf. Note that the
runtime profile is still retained as a Thrift structure as Impala
clients will still fetch query profiles using Thrift RPCs. This also
avoids duplicating the serialization implementation in both Thrift
and Protobuf for the runtime profile. The Thrift runtime profiles
are serialized and sent as a sidecar in ReportExecStatus() RPC.

This patch also fixes IMPALA-7241 which may lead to duplicated
dml stats being applied. The fix is by adding a monotonically
increasing version number for fragment instances' reports. The
coordinator will ignore any report smaller than or equal to the
version in the last report.

Testing done:
1. Exhaustive build.
2. Added some targeted test cases for profile serialization failure
   and RPC retries/timeout.

Change-Id: I7638583b433dcac066b87198e448743d90415ebe
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/common/global-flags.cc
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-writer.cc
M be/src/exec/hdfs-table-writer.h
M be/src/rpc/CMakeLists.txt
M be/src/rpc/jni-thrift-util.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/rpc-mgr.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.h
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/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/dml-exec-state.cc
M be/src/runtime/dml-exec-state.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/service/CMakeLists.txt
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
A be/src/service/control-service.cc
A be/src/service/control-service.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
A be/src/util/error-util-internal.h
M be/src/util/error-util-test.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
M be/src/util/runtime-profile.cc
M be/src/util/uid-util.h
M common/protobuf/CMakeLists.txt
M common/protobuf/common.proto
A common/protobuf/control_service.proto
M common/protobuf/data_stream_service.proto
M common/protobuf/row_batch.proto
M common/protobuf/rpc_test.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_rpc_timeout.py
65 files changed, 1,299 insertions(+), 769 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10855/20
--
To view, visit http://gerrit.cloudera.org:8080/10855
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 20
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: 

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11615 )

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11615/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11615/5//COMMIT_MSG@14
PS5, Line 14: coordinator backend states. In addition, due to the lack
: of coordinator between query fragment instances, a query may end
: without collecting the profiles from all fragment instances when
: one of them hits an error before some other fragment instance 
manages
: to finish calling Prepare(), leading to missing
> Does this mean that we've fixed the problem in IMPALA-7148 and can re-enabl
Yes. Uncommented some of the tests in bloom_filters.test.


http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.cc@236
PS1, Line 236: if (overall_status_.IsCancelled()) {
> Right, I was thinking that CANCELLED_INTERNALLY shouldn't leak out of fragm
Keep it as-is for now.


http://gerrit.cloudera.org:8080/#/c/11615/5/testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test
File 
testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test:

http://gerrit.cloudera.org:8080/#/c/11615/5/testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test@18
PS5, Line 18:  QUERY
> This test doesn't really have anything to do with nondeterminism. Would you
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 25 Oct 2018 02:13:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Hello Thomas Marshall, Tim Armstrong, Joe McDonnell, Bikramjeet Vig, Impala 
Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/11615

to look at the new patch set (#6).

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..

IMPALA-4063: Merge report of query fragment instances per executor

Previously, each fragment instance executing on an executor will
independently report its status to the coordinator periodically.
This creates a huge amount of RPCs to the coordinator under highly
concurrent workloads, causing lock contention in the coordinator's
backend states when multiple fragment instances send them at the
same time. In addition, due to the lack of coordination between query
fragment instances, a query may end without collecting the profiles
from all fragment instances when one of them hits an error before
another fragment instance manages to finish Prepare(), leading to
missing profiles for certain fragment instances.

This change fixes the problem above by making a thread per QueryState
(started by QueryExecMgr) to be responsible for periodically reporting
the status and profiles of all fragment instances of a query running
on a backend. As part of this refactoring, each query fragment instance
will not report their errors individually. Instead, there is a cumulative
status maintained per QueryState. It's set to the error status of the first
fragment instance which hits an error or any general error (e.g. failure
to start a thread) when starting fragment instances. With this change,
the status reporting threads are also removed.

Testing done: exhaustive tests

This patch is based on a patch by Sailesh Mukil

Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M common/protobuf/control_service.proto
M common/thrift/RuntimeProfile.thrift
M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test
A testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test
D 
testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
21 files changed, 419 insertions(+), 473 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/11615/6
--
To view, visit http://gerrit.cloudera.org:8080/11615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10855/19/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/10855/19/be/src/runtime/query-state.cc@363
PS19, Line 363: LOG(DFATAL) << FromKuduStatus(sidecar_status, "Failed 
to add sidecar").GetDetail();
> line too long (91 > 90)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 25 Oct 2018 02:12:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10855 )

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..


Patch Set 20: Code-Review+2

Carry +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 20
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 25 Oct 2018 02:12:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 18:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11535/18/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
File testdata/workloads/functional-query/queries/QueryTest/aggregation.test:

http://gerrit.cloudera.org:8080/#/c/11535/18/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1402
PS18, Line 1402: bigint, float
You forgot to update result type for the extra column in the select list.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 18
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 25 Oct 2018 02:17:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 17: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 17
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 24 Oct 2018 21:44:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7727: Fix TStatusCode to TErrorCode mapping

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11778 )

Change subject: IMPALA-7727: Fix TStatusCode to TErrorCode mapping
..


Patch Set 4:

OK. Will do a pass tonight.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie62527734aa73c1524c731773638590bdac9e789
Gerrit-Change-Number: 11778
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 30 Oct 2018 00:23:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7727: Fix TStatusCode to TErrorCode mapping

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11778 )

Change subject: IMPALA-7727: Fix TStatusCode to TErrorCode mapping
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie62527734aa73c1524c731773638590bdac9e789
Gerrit-Change-Number: 11778
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 30 Oct 2018 06:00:21 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Hello Thomas Marshall, Tim Armstrong, Joe McDonnell, Bikramjeet Vig, Impala 
Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/11615

to look at the new patch set (#7).

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..

IMPALA-4063: Merge report of query fragment instances per executor

Previously, each fragment instance executing on an executor will
independently report its status to the coordinator periodically.
This creates a huge amount of RPCs to the coordinator under highly
concurrent workloads, causing lock contention in the coordinator's
backend states when multiple fragment instances send them at the
same time. In addition, due to the lack of coordination between query
fragment instances, a query may end without collecting the profiles
from all fragment instances when one of them hits an error before
another fragment instance manages to finish Prepare(), leading to
missing profiles for certain fragment instances.

This change fixes the problem above by making a thread per QueryState
(started by QueryExecMgr) to be responsible for periodically reporting
the status and profiles of all fragment instances of a query running
on a backend. As part of this refactoring, each query fragment instance
will not report their errors individually. Instead, there is a cumulative
status maintained per QueryState. It's set to the error status of the first
fragment instance which hits an error or any general error (e.g. failure
to start a thread) when starting fragment instances. With this change,
the status reporting threads are also removed.

Testing done: exhaustive tests

This patch is based on a patch by Sailesh Mukil

Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/control-service.cc
M be/src/service/control-service.h
M common/protobuf/control_service.proto
M common/thrift/RuntimeProfile.thrift
M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test
A testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test
D 
testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_observability.py
M tests/query_test/test_udfs.py
22 files changed, 420 insertions(+), 475 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/11615/7
--
To view, visit http://gerrit.cloudera.org:8080/11615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

[Michael Ho (Code Review)]

Hello Thomas Marshall, Todd Lipcon, Tim Armstrong, Bikramjeet Vig, Impala 
Public Jenkins, Michal Ostrowski,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10855

to look at the new patch set (#21).

Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use 
KRPC
..

IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

This change converts ReportExecStatus() RPC from thrift
based RPC to KRPC. This is done in part of the preparation
for fixing IMPALA-2990 as we can take advantage of TCP connection
multiplexing in KRPC to avoid overwhelming the coordinator
with too many connections by reducing the number of TCP connection
to one for each executor.

This patch also introduces a new service pool for all query execution
control related RPCs in the future so that control commands from
coordinators aren't blocked by long-running DataStream services' RPCs.
To avoid unnecessary delays due to sharing the network connections
between DataStream service and Control service, this change added the
service name as part of the user credentials for the ConnectionId
so each service will use a separate connection.

The majority of this patch is mechanical conversion of some Thrift
structures used in ReportExecStatus() RPC to Protobuf. Note that the
runtime profile is still retained as a Thrift structure as Impala
clients will still fetch query profiles using Thrift RPCs. This also
avoids duplicating the serialization implementation in both Thrift
and Protobuf for the runtime profile. The Thrift runtime profiles
are serialized and sent as a sidecar in ReportExecStatus() RPC.

This patch also fixes IMPALA-7241 which may lead to duplicated
dml stats being applied. The fix is by adding a monotonically
increasing version number for fragment instances' reports. The
coordinator will ignore any report smaller than or equal to the
version in the last report.

Testing done:
1. Exhaustive build.
2. Added some targeted test cases for profile serialization failure
   and RPC retries/timeout.

Change-Id: I7638583b433dcac066b87198e448743d90415ebe
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/common/global-flags.cc
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-writer.cc
M be/src/exec/hdfs-table-writer.h
M be/src/rpc/CMakeLists.txt
M be/src/rpc/jni-thrift-util.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/rpc-mgr.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.h
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/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/dml-exec-state.cc
M be/src/runtime/dml-exec-state.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/service/CMakeLists.txt
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
A be/src/service/control-service.cc
A be/src/service/control-service.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
A be/src/util/error-util-internal.h
M be/src/util/error-util-test.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
M be/src/util/runtime-profile.cc
M be/src/util/uid-util.h
M common/protobuf/CMakeLists.txt
M common/protobuf/common.proto
A common/protobuf/control_service.proto
M common/protobuf/data_stream_service.proto
M common/protobuf/row_batch.proto
M common/protobuf/rpc_test.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_rpc_timeout.py
65 files changed, 1,299 insertions(+), 769 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10855/21
--
To view, visit http://gerrit.cloudera.org:8080/10855
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 21
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: 

[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11615 )

Change subject: IMPALA-4063: Merge report of query fragment instances per 
executor
..


Patch Set 7: Code-Review+2

Carry +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 01 Nov 2018 16:56:21 +
Gerrit-HasComments: No