[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service 
queue
..


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9282/3//COMMIT_MSG@20
PS3, Line 20: 20
5%


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/service/data-stream-service.cc@41
PS3, Line 41: DEFINE_string(datastream_service_queue_mem_limit, "5%", 
queue_limit_msg.c_str());
5% seems ok to start with. Would be good to do some experiments to get an idea 
for what typical consumption is.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 16 Feb 2018 07:21:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5518: Allocate KrpcDataStreamRecvr RowBatch tuples from BufferPool

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9344 )

Change subject: IMPALA-5518: Allocate KrpcDataStreamRecvr RowBatch tuples from 
BufferPool
..


Patch Set 1:

(8 comments)

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

http://gerrit.cloudera.org:8080/#/c/9344/3//COMMIT_MSG@29
PS3, Line 29: Note that the proper reservation mechanism of the exchange node
I was thinking through the implications of allocating this memory through the 
buffer pool. One thing that I'm a little concerned about is the implications of 
having 80% of process memory for the buffer pool and 80% of each query's memory 
devoted to reserved buffer pool memory. If every query's reserved operators 
used up to the 80% they were allowed, it could squeeze out the exchanges. Or 
even if one large query was running with the full process mem limit, its 
spilling operators could squeeze out the exchanges.

I think it is probably uncommon to have systems running that close to capacity, 
but it's worth thinking about.

We could consider bumping the process-wide buffer pool limit to a higher value 
to allow headroom, e.g. 85%. With the scanners and this code switching to 
allocating from the buffer pool, we probably will have reduced non-buffer-pool 
memory requirements significantly, so maybe we can justify this.


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/exec/blocking-join-node.cc
File be/src/exec/blocking-join-node.cc:

http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/exec/blocking-join-node.cc@163
PS1, Line 163: // Release resources in 'build_batch_' before closing the 
children as some of the
I wonder if this was the root cause of IMPALA-5715.


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/exec/exchange-node.cc
File be/src/exec/exchange-node.cc:

http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/exec/exchange-node.cc@82
PS1, Line 82: state->exec_env()
Use ExecEnv::GetInstance() for consistency?


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/exec/exchange-node.cc@138
PS1, Line 138:   if (recvr_buffer_pool_client_.is_registered()) {
This check isn't necessary, deregistering an unregistered client is a no-op (by 
design so that cleanup code like this is simpler to get correct).


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:

http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@123
PS1, Line 123: BufferPool::BufferHandle* tuple_ptrs_buffer = 
&(tuple_ptrs_info_->buffer);
Why can't we allocate one buffer large enough for the tuple pointers and the 
data? Doing two allocations seems somewhat wasteful given the tuple pointers 
are quite small.

I don't think the separate lifetimes for the tuple_ptrs_ and actual data is 
necessary for this use case. The separate tuple_ptrs_ was an optimisation added 
for nested types that is designed to reduce the cost of Reset(), but we 
shouldn't feel bound to follow that pattern here for batches that are internal 
to the ExchangeNode/DataStreamRecvr. I don't think we need Reset() to preserve 
the tuple_ptrs_ for these batches, since they're discarded instead of reused. 
It looks like Reset() is only indirectly called via AcquireResourceOwnership() 
for those batches.


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@199
PS1, Line 199: std::unique_ptr* row_batch_ptr) {
nit: don't need the std:: prefixes in .cc generally


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@217
PS1, Line 217: tuple_ptrs_info_->client, &(tuple_ptrs_info_->buffer));
> That's a possibility. However, the unused reservation may be used for alloc
Yeah I think holding onto the reservation may reduce the number of possible 
failure points, e.g. if other operators eat up more memory during the query.


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@484
PS1, Line 484:   std::swap(tuple_ptrs_info_, src->tuple_ptrs_info_);
We don't need to support AcquireState() for these batches do we? I don't think 
they escape from the ExchangeNode. Might be simpler to just DCHECK on the 
operations that we don't allow.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4b1a45f68b9df0d3b539511e15aff15700246f2
Gerrit-Change-Number: 9344
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 16 Feb 2018 07:19:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5518: Allocate KrpcDataStreamRecvr RowBatch tuples from BufferPool

[Michael Ho (Code Review)]

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

Change subject: IMPALA-5518: Allocate KrpcDataStreamRecvr RowBatch tuples from 
BufferPool
..


Patch Set 3:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/9344/1//COMMIT_MSG@7
PS1, Line 7:
> KrpcDataStreamRecvr
Done


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:

http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.h@151
PS1, Line 151: FromPr
> How about calling it FromProtoBuf()?
Done


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:

http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@94
PS1, Line 94:
> why is this safe? because when allocating from malloc, we don't expect any
Yes, my intention is to not change the existing Thrift behavior which assumes 
allocation always succeeded. I agree that we should add a DCHECK to verify.


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@120
PS1, Line 120:   DCHECK_EQ(num_tuples_per_row_, 
row_desc_->tuple_descriptors().size());
 :   DCHECK_GT(tuple_ptrs_size_, 0);
 : }
 :
 : void RowBatch::Deserialize(const kudu::Slice& 
input_tuple_offsets,
 : const kudu::Slice& input_tuple_data, int64_t 
uncompressed_size,
 : bool is_compressed, uint8_t* tuple_data) {
 :   DCHECK(tuple_ptrs_ != nullptr);
 :   DCHECK(tuple_data != nullptr);
 :   if (is_compressed) {
 : // Decompress tuple data into data pool
 : const uint8_t* compressed_data = input_tuple_data.data();
 : size_t compressed_size = input_tuple_data.size();
 :
 : Lz4Decompressor decompressor(nullptr, false);
 :
> how about  moving this block of code into the callers? The caller would set
Good idea about moving this block into FromProtobuf().

I refrain from changing the Thrift related logic too much so I didn't implement 
FromThrift().


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@217
PS1, Line 217:   tuple_data_pool_.FreeAll();
> Should we return the reservation at this point (until we implement real res
That's a possibility. However, the unused reservation may be used for 
allocating buffers of new row batches. This avoids the need to traverse the 
reservation hierarchy and acquires the lock of the root tracker repeatedly when 
increasing and decreasing the reservation. The reservation of the client should 
only grow to peak usage of the exchange node only.


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@393
PS1, Line 393:   RETURN_IF_ERROR(
> shoudl we use MemTracker::MemLimitExceeded() so we get all the debug info?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4b1a45f68b9df0d3b539511e15aff15700246f2
Gerrit-Change-Number: 9344
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 16 Feb 2018 06:52:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5518: Allocate KrpcDataStreamRecvr RowBatch tuples from BufferPool

[Michael Ho (Code Review)]

Hello Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-5518: Allocate KrpcDataStreamRecvr RowBatch tuples from 
BufferPool
..

IMPALA-5518: Allocate KrpcDataStreamRecvr RowBatch tuples from BufferPool

Previously, tuple pointers of a row batch are allocated from
the heap via malloc() and tuple data is allocated from the
MemPool associated with the RowBatch. This change converts
the allocations of tuple pointers and tuple data to using
BufferPool for row batches allocated from KrpcDataStreamRecvr.
The primary motivation for this change is to take advantage of
the fact that buffers allocated from BufferPool always go back
to the per-core arena they came from when they are freed. This
alleviates the TCMalloc imbalance between the RPC service threads
and the fragment execution threads. As described in IMPALA-5518,
row batches are always allocated from the service threads' TCMalloc
cache and placed into the fragment execution threads' TCMalloc cache
when they're freed. This leads to underflow and overflow in those
threads' caches and high contention for the spinlock of the central
free list. With BufferPool, the memory always went back to its
originating arena so this kind of imbalance is less likely to occur.
This also dovetails with the long term plan to put most allocations
under BufferPool and have each operators in the plan reserved
appropriate amount of memory before execution.

Note that the proper reservation mechanism of the exchange node
hasn't yet been implemented in this change so the buffer pool client
handle used for allocating buffers has an ad-hoc set-up of no reservation
limit and using root reservation tracker as parent. This needs to be
fixed as part of IMPALA-6524. Part of this change also lowers the
minimum buffer size to 32KB to reduce amount of memory wastage as
a row batch's tuple pointers / tuple data can sometimes be smaller
than 64KB.

Testing done: Debug core build.

Change-Id: If4b1a45f68b9df0d3b539511e15aff15700246f2
---
M be/src/common/global-flags.cc
M be/src/exec/blocking-join-node.cc
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/partial-sort-node.cc
M be/src/runtime/data-stream-mgr-base.h
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.h
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/row-batch.cc
M be/src/runtime/row-batch.h
M be/src/service/query-options-test.cc
M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test
18 files changed, 218 insertions(+), 84 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If4b1a45f68b9df0d3b539511e15aff15700246f2
Gerrit-Change-Number: 9344
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5518: Allocate RowBatch tuples pointers and data from BufferPool

[Michael Ho (Code Review)]

Hello Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-5518: Allocate RowBatch tuples pointers and data from 
BufferPool
..

IMPALA-5518: Allocate RowBatch tuples pointers and data from BufferPool

Previously, tuple pointers of a row batch are allocated from
the heap via malloc() and tuple data is allocated from the
MemPool associated with the RowBatch. This change converts
the allocations of tuple pointers and tuple data to using
BufferPool for row batches allocated from KrpcDataStreamRecvr.
The primary motivation for this change is to take advantage of
the fact that buffers allocated from BufferPool always go back
to the per-core arena they came from when they are freed. This
alleviates the TCMalloc imbalance between the RPC service threads
and the fragment execution threads. As described in IMPALA-5518,
row batches are always allocated from the service threads' TCMalloc
cache and placed into the fragment execution threads' TCMalloc cache
when they're freed. This leads to underflow and overflow in those
threads' caches and high contention for the spinlock of the central
free list. With BufferPool, the memory always went back to its
originating arena so this kind of imbalance is less likely to occur.
This also dovetails with the long term plan to put most allocations
under BufferPool and have each operators in the plan reserved
appropriate amount of memory before execution.

Note that the proper reservation mechanism of the exchange node
hasn't yet been implemented in this change so the buffer pool client
handle used for allocating buffers has an ad-hoc set-up of no reservation
limit and using root reservation tracker as parent. This needs to be
fixed as part of IMPALA-6524. Part of this change also lowers the
minimum buffer size to 32KB to reduce amount of memory wastage as
a row batch's tuple pointers / tuple data can sometimes be smaller
than 64KB.

Testing done: Debug core build.

Change-Id: If4b1a45f68b9df0d3b539511e15aff15700246f2
---
M be/src/common/global-flags.cc
M be/src/exec/blocking-join-node.cc
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/partial-sort-node.cc
M be/src/runtime/data-stream-mgr-base.h
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.h
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/row-batch.cc
M be/src/runtime/row-batch.h
M be/src/service/query-options-test.cc
M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test
18 files changed, 218 insertions(+), 84 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If4b1a45f68b9df0d3b539511e15aff15700246f2
Gerrit-Change-Number: 9344
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] Bump Kudu version to 0eef8e0

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9348 )

Change subject: Bump Kudu version to 0eef8e0
..


Patch Set 1: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id983ceb8806b2a002fadb19c065267d45f4c65db
Gerrit-Change-Number: 9348
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 16 Feb 2018 06:36:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] Bump Kudu version to 0eef8e0

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9348 )

Change subject: Bump Kudu version to 0eef8e0
..

Bump Kudu version to 0eef8e0

This pulls in support for DECIMAL.

Change-Id: Id983ceb8806b2a002fadb19c065267d45f4c65db
Reviewed-on: http://gerrit.cloudera.org:8080/9348
Reviewed-by: Lars Volker 
Reviewed-by: Grant Henke 
Tested-by: Impala Public Jenkins
---
M be/src/exec/kudu-util.cc
M bin/impala-config.sh
2 files changed, 5 insertions(+), 2 deletions(-)

Approvals:
  Lars Volker: Looks good to me, approved
  Grant Henke: Looks good to me, but someone else must approve
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id983ceb8806b2a002fadb19c065267d45f4c65db
Gerrit-Change-Number: 9348
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service 
queue
..


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/rpc/impala-service-pool.cc@155
PS2, Line 155: CheckLimitExceeded
Why don't we call AnyLimitExceeded()? If we're over the process limit it seems 
important to reject incoming RPCs.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/impala-service-pool.cc@159
PS3, Line 159:   c->DiscardTransfer();
Maybe drop the lock at this point to avoid calling free() while holding the 
lock? Might help in some cases where system is already overloaded.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h
File be/src/rpc/rpc-mgr-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h@135
PS3, Line 135: TakeoverService
nit: take over is two words, so TakeOverService


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h@234
PS3, Line 234:   dynamic_cast(ping_impl)->mem_tracker()));
dynamic_cast<> here and below shouldn't be necessary - don't need to check 
runtime type so static_cast should work.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test.cc@186
PS3, Line 186: dynamic_cast
static_cast should also work here and below.


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

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc@176
PS2, Line 176:   service_mem_tracker_->Release(transfer_size);
> The process limit is checked by looking at TC-malloc and the buffer pool, r
I think generally we should be conservative and call Consume() before 
Release(). I can see Michael's argument in this case we could cause unnecessary 
failures in that way. I think in either case the probability of problems is low 
so this seems ok.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/runtime/mem-tracker.h@281
PS3, Line 281:   bool CheckLimitExceeded() const { return limit_ >= 0 && limit_ 
< consumption(); }
Why not use the existing public LimitExceeded() or AnyLimitExceeded() APIs. It 
should also work for this purpose. I think having three similar public APIs is 
really confusing.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 16 Feb 2018 06:31:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6258: Uninitialized tuple pointers in row batch for empty rows

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9239 )

Change subject: IMPALA-6258: Uninitialized tuple pointers in row batch for 
empty rows
..


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1947/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2981227e62eb5971508e0698e189519755de
Gerrit-Change-Number: 9239
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 16 Feb 2018 06:03:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6258: Uninitialized tuple pointers in row batch for empty rows

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9239 )

Change subject: IMPALA-6258: Uninitialized tuple pointers in row batch for 
empty rows
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2981227e62eb5971508e0698e189519755de
Gerrit-Change-Number: 9239
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 16 Feb 2018 06:03:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9291 )

Change subject: IMPALA-6508: add KRPC test flag
..


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1946/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 05:51:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[David Knupp (Code Review)]

David Knupp has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9291 )

Change subject: IMPALA-6508: add KRPC test flag
..


Patch Set 10: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 05:31:57 +
Gerrit-HasComments: No

[Impala-ASF-CR] Bump Kudu Java version to 1.7.0

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9349 )

Change subject: Bump Kudu Java version to 1.7.0
..


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1945/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69a9bc9e36657a936591f723cf4110a1297973ab
Gerrit-Change-Number: 9349
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 16 Feb 2018 05:03:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] Bump Kudu Java version to 1.7.0

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9349 )

Change subject: Bump Kudu Java version to 1.7.0
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69a9bc9e36657a936591f723cf4110a1297973ab
Gerrit-Change-Number: 9349
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 16 Feb 2018 04:53:50 +
Gerrit-HasComments: No

[Impala-ASF-CR] Bump Kudu Java version to 1.7.0

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9349 )

Change subject: Bump Kudu Java version to 1.7.0
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69a9bc9e36657a936591f723cf4110a1297973ab
Gerrit-Change-Number: 9349
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Grant Henke 
Gerrit-Comment-Date: Fri, 16 Feb 2018 04:52:40 +
Gerrit-HasComments: No

[Impala-ASF-CR] Bump Kudu version to 0eef8e0

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9348 )

Change subject: Bump Kudu version to 0eef8e0
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id983ceb8806b2a002fadb19c065267d45f4c65db
Gerrit-Change-Number: 9348
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 16 Feb 2018 04:52:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] Bump Kudu Java version to 1.7.0

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9349


Change subject: Bump Kudu Java version to 1.7.0
..

Bump Kudu Java version to 1.7.0

Change-Id: I69a9bc9e36657a936591f723cf4110a1297973ab
---
M bin/impala-config.sh
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I69a9bc9e36657a936591f723cf4110a1297973ab
Gerrit-Change-Number: 9349
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5752: Add support for DECIMAL on Kudu tables

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9306 )

Change subject: IMPALA-5752: Add support for DECIMAL on Kudu tables
..


Patch Set 2:

(5 comments)

> (1 comment)
 >
 > I ran the code formatter and updated the patch with those changes.
 >
 > I think the runtime filter functionality should be added, but I
 > should likely do that in a separate patch to keep this patch
 > smaller and focused. I can create a jira to track that if you
 > agree. I will look how difficult adding decimal min-max filters is
 > tomorrow.

Sounds good.

I think the biggest thing now is just making sure we've tested everything well 
enough. For example, you could add tests for insert/update/delete with decimal 
values that are outside the range for the precision/scale of the column, 
creating a table with a decimal primary key, etc.

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

http://gerrit.cloudera.org:8080/#/c/9306/2//COMMIT_MSG@10
PS2, Line 10:
We generally include a "Testing" section here, eg. what tests were added, what 
did you run, etc.

You should also take a look at tests/query_tests/test_decimal_queries.py. You 
should be able to add Kudu to TestDecimalQueries with something like:
"v.get_value('table_format').file_format in ['parquet', 'kudu']"

You'll also need to take a look at 
testdata/datasets/functional/schema_constraints.csv to add the decimal related 
tables to data load for Kudu.


http://gerrit.cloudera.org:8080/#/c/9306/1/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:

http://gerrit.cloudera.org:8080/#/c/9306/1/be/src/exec/kudu-util.h@a69
PS1, Line 69:
> Since it's not used I don't see a reason to keep it around.
Well its useful for debugging


http://gerrit.cloudera.org:8080/#/c/9306/2/be/src/exec/kudu-util.cc
File be/src/exec/kudu-util.cc:

http://gerrit.cloudera.org:8080/#/c/9306/2/be/src/exec/kudu-util.cc@214
PS2, Line 214: return ColumnType(PrimitiveType::TYPE_DECIMAL);
I imagine the formatter told you to do this. We don't follow it 100%, so you 
can just follow the example of the above lines.


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

http://gerrit.cloudera.org:8080/#/c/9306/2/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@625
PS2, Line 625: // TODO: Support Kudu Decimal Min/Max Filters
I filed IMPALA-6533. Can you reference that here?


http://gerrit.cloudera.org:8080/#/c/9306/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java
File fe/src/main/java/org/apache/impala/util/KuduUtil.java:

http://gerrit.cloudera.org:8080/#/c/9306/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@400
PS2, Line 400: return org.apache.kudu.Type.DECIMAL;
Another place you can ignore the formatter.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f927fce25dc4fa9529a4e0b688825699f5c0ea6
Gerrit-Change-Number: 9306
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 16 Feb 2018 04:02:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9291 )

Change subject: IMPALA-6508: add KRPC test flag
..


Patch Set 10:

(1 comment)

Thanks for the reviews. Please see PS10.

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

http://gerrit.cloudera.org:8080/#/c/9291/9/bin/run-all-tests.sh@56
PS9, Line 56: # Extra arguments passed to start-impala-cluster for tests. These 
do not apply to custom
: # cluster tests.
> Can you add a comment that this does not apply to custom cluster tests?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 03:43:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[Lars Volker (Code Review)]

Hello Michael Ho, Michael Brown, Sailesh Mukil, David Knupp,

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

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

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

Change subject: IMPALA-6508: add KRPC test flag
..

IMPALA-6508: add KRPC test flag

This change adds a flag "--use_krpc" to start-impala-cluster.py. The
flag is currently passed as an argument to the impalad daemon. In the
future it will also enable KRPC for the catalogd and statestored
daemons.

This change also adds a flag "--test_krpc" to pytest. When running tests
using "impala-py.test --test_krpc", the test cluster will be started
by passing "--use_krpc" to start-impala-cluster.py (see above).

This change also adds a SkipIf to skip tests based on whether the
cluster was started with KRPC support or not.

- SkipIf.not_krpc can be used to mark a test that depends on KRPC.
- SkipIf.not_thrift can be used to mark a test that depends on Thrift
  RPC.

This change adds a meta test to make sure that the new SkipIf decorators
work correctly. The test should be removed as soon as real tests have
been added with the new decorators.

Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
---
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
A tests/common/test_skip.py
M tests/conftest.py
M tests/custom_cluster/test_krpc_mem_usage.py
M tests/custom_cluster/test_rpc_exception.py
8 files changed, 86 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/9291/10
--
To view, visit http://gerrit.cloudera.org:8080/9291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9292 )

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..


Patch Set 7:

(10 comments)

Thanks for the reviews. Please see my inline comments and PS7.

http://gerrit.cloudera.org:8080/#/c/9292/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9292/6//COMMIT_MSG@9
PS6, Line 9: /rpcz debug web page
> If possible, could you post a link to a screenshot of a /rpcz page with thi
Done


http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h@36
PS6, Line 36: // A pool of threads that handle new incoming RPC calls.
: // Also includes a queue that calls get pushed onto for handling 
by the pool.
: class ImpalaServicePool : public kudu::rpc::RpcService {
:  public:
:   ImpalaServicePool(MemTracker* mem_tracker,
:
> My preference would be to expose the queue overflow metrics per service. In
Done


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.h@48
PS5, Line 48:
> Please document the input arguments.
I reverted this change.


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225: // Release the InboundCal
> My understanding is that this only tracks the number of threads running the
Thanks for the explanations. The total count of RPC calls is already exposed in 
the histograms so we should be good.


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

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.h@168
PS6, Line 168: }
> Please see comments elsewhere. This may not be needed.
Done


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

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@100
PS5, Line 100: ficates, and encrypti
> Can you please add a comment for 'metrics' and 'use_tls' (not your change)
Done


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.cc@85
PS5, Line 85: TITY_server.Instant
> This is a shared counter across all RPC services, right ? If so, please add
Removed, N/A.


http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc@82
PS6, Line 82:
> As discussed in person, it may be useful to have this in ExecEnv instead. F
I moved this to the ExecEnv. Moving the other RPC metrics into the same group 
seems to be a compatibility breaking change since various external tooling 
might depend on them being in impala-server. We should track moving them in a 
separate Jira.


http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc@198
PS6, Line 198:   document->AddMember("num_inbound_calls_in_flight", 
num_inbound_calls_in_flight,
 :   document->GetAllocator());
 :
> How often is this run ? I assume this only acquires the shared lock so shou
This runs every time the /rpcz page gets rendered. Yes, QueueInboundCall() only 
takes a shared lock as well.


http://gerrit.cloudera.org:8080/#/c/9292/6/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/9292/6/common/thrift/metrics.json@1639
PS6, Line 1639:   {
  : "description": "Service $0: Total number of incoming RPCs 
that were rejected due to overflow of the service queue.",
  : "contexts": [
  :   "IMPALAD"
  : ],
  : "label": "Service $0 Incoming RPC Queue Overflows",
  : "units": "UNIT",
  : "kind": "COUNTER",
  : "key": "rpc.$0.rpcs_queue_overflow"
  :   }
> Not needed anymore ?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 03:41:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Lars Volker (Code Review)]

Hello Michael Ho, Sailesh Mukil,

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

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

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

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..

IMPALA-6269: Expose KRPC metrics on debug webpage

This change exposes KRPC metrics on the /rpcz debug web page.

This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm

This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.

This change is based on a change by Sailesh Mukil.

TODO: testing, once IMPALA-6508 has been submitted.

Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
15 files changed, 387 insertions(+), 106 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[Michael Brown (Code Review)]

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

Change subject: IMPALA-6508: add KRPC test flag
..


Patch Set 9: Code-Review+1

(1 comment)

Feel free to carry forward my +1 after fixing. I'll let David +2 when he's 
happy.

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

http://gerrit.cloudera.org:8080/#/c/9291/9/bin/run-all-tests.sh@56
PS9, Line 56: # Extra arguments passed to start-impala-cluster for tests
: : ${TEST_START_CLUSTER_ARGS:=}
Can you add a comment that this does not apply to custom cluster tests?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 9
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 03:01:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Bump Kudu version to 0eef8e0

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9348 )

Change subject: Bump Kudu version to 0eef8e0
..


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1944/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id983ceb8806b2a002fadb19c065267d45f4c65db
Gerrit-Change-Number: 9348
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 16 Feb 2018 02:54:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] Bump Kudu version to 0eef8e0

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9348 )

Change subject: Bump Kudu version to 0eef8e0
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id983ceb8806b2a002fadb19c065267d45f4c65db
Gerrit-Change-Number: 9348
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 16 Feb 2018 02:53:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] Bump Kudu version to 0eef8e0

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9348


Change subject: Bump Kudu version to 0eef8e0
..

Bump Kudu version to 0eef8e0

This pulls in support for DECIMAL.

Change-Id: Id983ceb8806b2a002fadb19c065267d45f4c65db
---
M be/src/exec/kudu-util.cc
M bin/impala-config.sh
2 files changed, 5 insertions(+), 2 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id983ceb8806b2a002fadb19c065267d45f4c65db
Gerrit-Change-Number: 9348
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 

[native-toolchain-CR] PREVIEW: build ORC C++ lib in toolchain

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9274 )

Change subject: PREVIEW: build ORC C++ lib in toolchain
..


Patch Set 3:

Thanks for giving it a try! I might try to see if the 
https://issues.apache.org/jira/browse/ORC-266 patch applies to the 1.4.3 
release. If it does then I can remove my custom patch.


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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9274
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 16 Feb 2018 02:15:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4874: Increase maximum KRPC message size

[Michael Ho (Code Review)]

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

Change subject: IMPALA-4874: Increase maximum KRPC message size
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9337/1/be/src/kudu/rpc/transfer.cc@38
PS1, Line 38: INT_MAX
> Do we want to change it here? Or set it in the RpcMgr constructor (less cod
I second Sailesh's recommendation to do

FLAGS_rpc_max_message_size = INT_MAX; in RpcMgr before we construct the 
messenger.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I876bba0536e1d85e41eacd9c0aeccfe5c2126e58
Gerrit-Change-Number: 9337
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:36:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [DOCS] Fix in REPLICA PREFERENCE numeric options

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9341 )

Change subject: [DOCS] Fix in REPLICA_PREFERENCE numeric options
..


Patch Set 3: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-docs-submit/195/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia10e69ac38229e0969db11b7edbcf08c2444602b
Gerrit-Change-Number: 9341
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:17:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] [DOCS] Fix in REPLICA PREFERENCE numeric options

[Alex Rodoni (Code Review)]

Alex Rodoni has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9341


Change subject: [DOCS] Fix in REPLICA_PREFERENCE numeric options
..

[DOCS] Fix in REPLICA_PREFERENCE numeric options

Change-Id: Ia10e69ac38229e0969db11b7edbcf08c2444602b
---
M docs/topics/impala_replica_preference.xml
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia10e69ac38229e0969db11b7edbcf08c2444602b
Gerrit-Change-Number: 9341
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Michael Ho (Code Review)]

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

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..


Patch Set 6:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h@36
PS6, Line 36: // RPC metrics that are shared across all service pools. They 
need to be passed into the
: // service pool constructor and will aggregate values across all 
pools.
: struct RpcMetrics {
:   // Number of RPCs that were rejected due to the queue being 
full. Not owned.
:   IntCounter* rpcs_queue_overflow = nullptr;
: };
> Happy to remove it, what do others think?
My preference would be to expose the queue overflow metrics per service. In the 
end, we probably won't have many different RPC services (most likely 2). One 
for data stream related RPC and one for control command related (e.g. status 
report, starting fragment execution).

While having an aggregate count across all services is still a reasonable 
proxy, exposing them per service makes it easier to identify the problematic 
one and it shouldn't be too bad if we don't plan to have a lot of services.


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225:
> Removed it. Can you clarify what asynchronously means here? Are you concern
My understanding is that this only tracks the number of threads running the RPC 
handler. If I understand Sailesh correctly, what he meant is that some payloads 
may be handled in deferred manner as the row batch queue fills up in the 
receiver.

In some sense, this may be useful if all the threads are stuck in handlers for 
a long time. However, one really cannot tell between whether the threads are 
stuck or they are always busy. May be a more meaningful metrics is the 
aggregate number of RPC calls handled so at least we know if the service thread 
is stuck or not.


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

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.h@168
PS6, Line 168: RpcMetrics rpc_metrics_;
Please see comments elsewhere. This may not be needed.


http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc@82
PS6, Line 82: MetricGroup* rpc_group = metrics->GetOrCreateChildGroup("rpc");
As discussed in person, it may be useful to have this in ExecEnv instead. For 
KRPC milestone 1, we still have a mix of KRPC and Thrift RPCs so it may be 
useful to move those existing Thrift RPC related metrics under this new group 
too.


http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc@198
PS6, Line 198:   // Add messenger metrics.
 :   DumpRunningRpcsResponsePB response;
 :   messenger_->DumpRunningRpcs(DumpRunningRpcsRequestPB(), 
);
How often is this run ? I assume this only acquires the shared lock so 
shouldn't block enqueuing incoming RPCs, right ?


http://gerrit.cloudera.org:8080/#/c/9292/6/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/9292/6/common/thrift/metrics.json@1639
PS6, Line 1639:   {
  : "description": "Total number of incoming RPCs that timed 
out in the incoming service queue.",
  : "contexts": [
  :   "IMPALAD"
  : ],
  : "label": "rpcs-timed-out-in-queue",
  : "units": "UNIT",
  : "kind": "COUNTER",
  : "key": "rpc.rpcs-timed-out-in-queue"
  :   },
Not needed anymore ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:05:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9339 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..

IMPALA-6405: Error when string to decimal cast overflows

Before this patch, when there was an error when converting a string to
a decimal, a NULL was returned. In this patch, we change this behavior
so that an return in error is returned if decimal_v2 is enabled. We also
add a warning if there is an underflow.

The reasoning is that we want stricter behavior in decimal_v2.

Testing:
- Added some EE tests.
- Ran an exhaustive build, which passed.

Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 81 insertions(+), 15 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9339 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9339/1//COMMIT_MSG@10
PS1, Line 10: a decimal, a MULL was returned. In this patch, we change this 
behavior
> NULL
oops, done.


http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
File fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java:

http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@85
PS1, Line 85: if (resultOfReverseCast != null &&
> It seems weird (at least to me) that LiteralExpr.create would return null i
No, other casts do not raise errors.

LiteralExpr.create() returns a null when there is an error or a warning 
according to the comment in the Java file. To me this does not seem weird.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:05:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9292 )

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225:
> Removed it. Can you clarify what asynchronously means here? Are you concern
One example is that in the KrpcDataStreamRecvr, we can defer responding to RPCs 
for early senders. So the RPC is technically not handled until the RPC is 
responded to. However in this case, the 'num_in_handlers_' wouldn't reflect the 
number of RPCs being handled correctly.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 00:43:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9292 )

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..


Patch Set 6:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h@36
PS6, Line 36: // RPC metrics that are shared across all service pools. They 
need to be passed into the
: // service pool constructor and will aggregate values across all 
pools.
: struct RpcMetrics {
:   // Number of RPCs that were rejected due to the queue being 
full. Not owned.
:   IntCounter* rpcs_queue_overflow = nullptr;
: };
> Do we need this now since we only have one service pool? My take is that we
Happy to remove it, what do others think?


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225:
> The RPC handler can run asynchronously, so we can't accurately track the nu
Removed it. Can you clarify what asynchronously means here? Are you concerned 
that requests could get queued up in the service?


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@236
PS5, Line 236:  return
> DCHECK_EQ
Done


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@238
PS5, Line 238:
> Wouldn't microseconds be too granular for the webpages?
This tells the printer how to interpret values in the histogram. Then they will 
be formatted to human readable strings.


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@256
PS5, Line 256: document->GetAllocator());
 : value->AddMember("incoming_que
> We'll have to remove this.
Done


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

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@127
PS5, Line 127: GeneratedServiceIf
> Why this change ? Is it because of something that's defined in GeneratedSer
We need to access GeneratedServiceIf::methods_by_name(), which is not in 
ServiceIf.


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@156
PS5, Line 156: void ToJson(rapidjson::Document* document);
> A quick comment on what's included in the output would help.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 00:22:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4874: Increase maximum KRPC message size

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9337 )

Change subject: IMPALA-4874: Increase maximum KRPC message size
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9337/1/be/src/kudu/rpc/transfer.cc@38
PS1, Line 38: INT_MAX
Do we want to change it here? Or set it in the RpcMgr constructor (less code 
divergence this way)?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I876bba0536e1d85e41eacd9c0aeccfe5c2126e58
Gerrit-Change-Number: 9337
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 16 Feb 2018 00:12:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9292 )

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9292/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9292/6//COMMIT_MSG@9
PS6, Line 9: /rpcz debug web page
If possible, could you post a link to a screenshot of a /rpcz page with this 
change?

It'll be easier to review the HTML and JS part with a visual.


http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h@36
PS6, Line 36: // RPC metrics that are shared across all service pools. They 
need to be passed into the
: // service pool constructor and will aggregate values across all 
pools.
: struct RpcMetrics {
:   // Number of RPCs that were rejected due to the queue being 
full. Not owned.
:   IntCounter* rpcs_queue_overflow = nullptr;
: };
Do we need this now since we only have one service pool? My take is that we can 
add it as necessary, since we already have a counter to track the same thing 
within the ServicePool.


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225:
The RPC handler can run asynchronously, so we can't accurately track the 
num_in_handlers from here. So, it'd be better to remove this.


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@238
PS5, Line 238:
Wouldn't microseconds be too granular for the webpages?


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@256
PS5, Line 256: document->GetAllocator());
 : value->AddMember("incoming_que
We'll have to remove this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:48:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9346


Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and 
related fns
..

IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

Before this patch, the output type of round() ceil() floor() trunc() was
not always the same as the input type. It was also inconsistent in
general. For example, round(double) returned an integer, but
round(double, int) returned a double.

After looking at other database systems, we decided that the guideline
should be that the output type should be the same as the input type. In
this patch, we change the behavior of the previously mentioned functions
so that if a double is given then a double is returned.

We also modify the rounding behavior to always round away from zero.
Before, we were rounding towards positive infinity in some cases.

Testinging:
- Updated tests
- Ran an exhaustive build which passed.

Cherry-picks: not for 2.x

Change-Id: I77541678012edab70b182378b11ca8753be53f97
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 77 insertions(+), 70 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Michael Ho (Code Review)]

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

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..


Patch Set 5:

(6 comments)

Some initial comments. Doing another pass.

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.h@48
PS5, Line 48:  public:
Please document the input arguments.


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@236
PS5, Line 236:  DCHECK(
DCHECK_EQ


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

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@100
PS5, Line 100: MetricGroup* metrics,
Can you please add a comment for 'metrics' and 'use_tls' (not your change) ?


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@127
PS5, Line 127: GeneratedServiceIf
Why this change ? Is it because of something that's defined in 
GeneratedServiceIf but not in the base class ServiceIf ?


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@156
PS5, Line 156: void ToJson(rapidjson::Document* document);
A quick comment on what's included in the output would help.


http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.cc@85
PS5, Line 85: rpcs_queue_overflow
This is a shared counter across all RPC services, right ? If so, please add a 
comment about that.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:37:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9292 )

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..


Patch Set 6:

PS 6 rebases the change and removed the client timeout metrics (as discussed 
between Michael and Lars in person). Our current client implementation will 
never time out and thus this value would always be 0.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:36:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage

[Lars Volker (Code Review)]

Hello Michael Ho, Sailesh Mukil,

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

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

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

Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
..

IMPALA-6269: Expose KRPC metrics on debug webpage

This change exposes KRPC metrics on the /rpcz debug web page.

This change also exposes metrics for rejected RPCs on the /metrics debug
web page.

This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.

This change is based on a change by Sailesh Mukil.

TODO: testing, once IMPALA-6508 has been submitted.

Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
15 files changed, 415 insertions(+), 107 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5518: Allocate RowBatch tuples pointers and data from BufferPool

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9344 )

Change subject: IMPALA-5518: Allocate RowBatch tuples pointers and data from 
BufferPool
..


Patch Set 1:

(7 comments)

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

http://gerrit.cloudera.org:8080/#/c/9344/1//COMMIT_MSG@7
PS1, Line 7:
KrpcDataStreamRecvr
(otherwise, it sounds like you are doing the full conversion)


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:

http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.h@151
PS1, Line 151: Create
How about calling it FromProtoBuf()?


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:

http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@a138
PS1, Line 138:
lol


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@94
PS1, Line 94: discard_result
why is this safe? because when allocating from malloc, we don't expect any 
error Status to be propagated? Let's make that explicit, e.g.:

Status status = Deserialize();
DCHECK(status.ok()) << "Error propagated only when using BufferPool";


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@120
PS1, Line 120:   if (client != nullptr) {
 : tuple_ptrs_info_.reset(new BufferInfo());
 : tuple_ptrs_info_->client = client;
 : BufferPool::BufferHandle* tuple_ptrs_buffer = 
&(tuple_ptrs_info_->buffer);
 : RETURN_IF_ERROR(AllocateBuffer(client, tuple_ptrs_size_, 
tuple_ptrs_buffer));
 : tuple_ptrs_ = 
reinterpret_cast(tuple_ptrs_buffer->data());
 :
 : BufferPool::BufferHandle tuple_data_buffer;
 : RETURN_IF_ERROR(AllocateBuffer(client, uncompressed_size, 
_data_buffer));
 : tuple_data = tuple_data_buffer.data();
 : AddBuffer(client, move(tuple_data_buffer), 
FlushMode::NO_FLUSH_RESOURCES);
 :   } else {
 : mem_tracker_->Consume(tuple_ptrs_size_);
 : tuple_ptrs_ = 
reinterpret_cast(malloc(tuple_ptrs_size_));
 : tuple_data = tuple_data_pool_.Allocate(uncompressed_size);
 :   }
how about  moving this block of code into the callers? The caller would set up 
the allocation however it wants and then call Deserialize() to fill it in.

It would also be nice to introduce a FromThrift() to have symmetry with the PB 
case, but you can ignore that if you want since that code will go away at some 
point.


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@217
PS1, Line 217: tuple_ptrs_info_->client, &(tuple_ptrs_info_->buffer));
Should we return the reservation at this point (until we implement real 
reservations)?

Or implement BufferPool::FreeUnreservedBuffer() to go allow with 
AllocateUnreservedBuffer(), and have that do it?


http://gerrit.cloudera.org:8080/#/c/9344/1/be/src/runtime/row-batch.cc@393
PS1, Line 393: "Failed to allocate $0 bytes buffer for row batch", 
buffer_len));
shoudl we use MemTracker::MemLimitExceeded() so we get all the debug info?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4b1a45f68b9df0d3b539511e15aff15700246f2
Gerrit-Change-Number: 9344
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:23:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9339 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9339/1//COMMIT_MSG@10
PS1, Line 10: a decimal, a MULL was returned. In this patch, we change this 
behavior
NULL


http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
File fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java:

http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@85
PS1, Line 85: if (resultOfReverseCast != null &&
It seems weird (at least to me) that LiteralExpr.create would return null 
instead of NullLiteral, and probably simpler to just make this the case even if 
errors are raised during the cast.

It also seems strange that we would only hit this with decimal.  Don't other 
casts also raise errors?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:19:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4874: Increase maximum KRPC message size

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9337 )

Change subject: IMPALA-4874: Increase maximum KRPC message size
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9337/1/be/src/kudu/rpc/transfer.cc@38
PS1, Line 38: INT_MAX
> LZ4 will fail to compress once we have uncompressed size of 2GB. That makes
Actually, the WriteVarint32ToArray takes a uint32_t, so just changing the Kudu 
code to uint32_t might be enough.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I876bba0536e1d85e41eacd9c0aeccfe5c2126e58
Gerrit-Change-Number: 9337
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 22:49:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5518: Allocate RowBatch tuples pointers and data from BufferPool

[Michael Ho (Code Review)]

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


Change subject: IMPALA-5518: Allocate RowBatch tuples pointers and data from 
BufferPool
..

IMPALA-5518: Allocate RowBatch tuples pointers and data from BufferPool

Previously, tuple pointers of a row batch are allocated from
the heap via malloc() and tuple data is allocated from the
MemPool associated with the RowBatch. This change converts
the allocations of tuple pointers and tuple data to using
BufferPool for row batches allocated from KrpcDataStreamRecvr.
The primary motivation for this change is to take advantage of
the fact that buffers allocated from BufferPool always go back
to the per-core arena they came from when they are freed. This
alleviates the TCMalloc imbalance between the RPC service threads
and the fragment execution threads. As described in IMPALA-5518,
row batches are always allocated from the service threads' TCMalloc
cache and placed into the fragment execution threads' TCMalloc cache
when they're freed. This leads to underflow and overflow in those
threads' caches and high contention for the spinlock of the central
free list. With BufferPool, the memory always went back to its
originating arena so this kind of imbalance is less likely to occur.
This also dovetails with the long term plan to put most allocations
under BufferPool and have each operators in the plan reserved
appropriate amount of memory before execution.

Note that the proper reservation mechanism of the exchange node
hasn't yet been implemented in this change so the buffer pool client
handle used for allocating buffers has an ad-hoc set-up of no reservation
limit and using root reservation tracker as parent. This needs to be
fixed as part of IMPALA-6524. Part of this change also lowers the
minimum buffer size to 32KB to reduce amount of memory wastage as
a row batch's tuple pointers / tuple data can sometimes be smaller
than 64KB.

Testing done: Debug core build.

Change-Id: If4b1a45f68b9df0d3b539511e15aff15700246f2
---
M be/src/common/global-flags.cc
M be/src/exec/blocking-join-node.cc
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/partial-sort-node.cc
M be/src/runtime/data-stream-mgr-base.h
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.h
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/row-batch.cc
M be/src/runtime/row-batch.h
M be/src/service/query-options-test.cc
M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test
18 files changed, 220 insertions(+), 83 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-4874: Increase maximum KRPC message size

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9337 )

Change subject: IMPALA-4874: Increase maximum KRPC message size
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9337/1/be/src/kudu/rpc/transfer.cc@38
PS1, Line 38: INT_MAX
> How close did you get to this in tests? Are we confident that values very c
LZ4 will fail to compress once we have uncompressed size of 2GB. That makes it 
hard to get to a compressed size near INT_MAX. I have definitely gone to 
>600MB, but that is not what you have in mind.

Looking at the code, I think you are right that there can be cases where 
sidecars approaching 2GB would trigger problems. I think this line could 
overflow at (2GB - a few bytes):
https://github.com/apache/kudu/blob/master/src/kudu/rpc/serialization.cc#L59

A couple options for fixes:
1. Use INT_MAX - 1KB (or some other constant). This gives us some leeway from 
any int32_t limit issues.
2. Fix the Kudu serialization code to use size_t in appropriate places. This 
doesn't really save us, because the WriteVarint32ToArray call wouldn't handle 
this anyway.


http://gerrit.cloudera.org:8080/#/c/9337/1/testdata/workloads/functional-query/queries/QueryTest/large_strings.test
File testdata/workloads/functional-query/queries/QueryTest/large_strings.test:

http://gerrit.cloudera.org:8080/#/c/9337/1/testdata/workloads/functional-query/queries/QueryTest/large_strings.test@220
PS1, Line 220: select cast(fnv_hash(l_comment) as string) as h from 
tpch_parquet.lineitem union all
> Did you check how well these compress? Each hash seems to occur 5 times whi
I will change this to salt it.

The total string sizes combine to about 1.2GB and I'm seeing message sizes 
around 300-400MB. I think the duplicate hash values are so far apart that the 
compression can't do much with it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I876bba0536e1d85e41eacd9c0aeccfe5c2126e58
Gerrit-Change-Number: 9337
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 22:34:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6230: Change output type of round() to match the input type

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9174


Change subject: IMPALA-6230: Change output type of round() to match the input 
type
..

IMPALA-6230: Change output type of round() to match the input type

Cherry-picks: not for 2.x

Change-Id: I202dc7123479b781094c029353197185fd0fc793
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 71 insertions(+), 64 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I202dc7123479b781094c029353197185fd0fc793
Gerrit-Change-Number: 9174
Gerrit-PatchSet: 8
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9339


Change subject: IMPALA-6405: Error when string to decimal cast overflows
..

IMPALA-6405: Error when string to decimal cast overflows

Before this patch, when there was an error when converting a string to
a decimal, a MULL was returned. In this patch, we change this behavior
so that an return in error is returned if decimal_v2 is enabled. We also
add a warning if there is an underflow.

The reasoning is that we want stricter behavior in decimal_v2.

Testing:
- Added some EE tests.
- Ran an exhaustive build, which passed.

Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 81 insertions(+), 15 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[native-toolchain-CR] Bump Kudu version to 0eef8e0

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9338 )

Change subject: Bump Kudu version to 0eef8e0
..


Patch Set 2: Verified+1


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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I186317c3440e1a50fffa87e67b23166da7715e1a
Gerrit-Change-Number: 9338
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 15 Feb 2018 22:07:25 +
Gerrit-HasComments: No

[native-toolchain-CR] Bump Kudu version to 0eef8e0

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9338 )

Change subject: Bump Kudu version to 0eef8e0
..

Bump Kudu version to 0eef8e0

This pulls in support for DECIMAL.

Change-Id: I186317c3440e1a50fffa87e67b23166da7715e1a
---
M buildall.sh
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Lars Volker: Looks good to me, approved
  Thomas Tauber-Marshall: Verified

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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I186317c3440e1a50fffa87e67b23166da7715e1a
Gerrit-Change-Number: 9338
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[native-toolchain-CR] Bump Kudu version to 0eef8e0

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9338 )

Change subject: Bump Kudu version to 0eef8e0
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I186317c3440e1a50fffa87e67b23166da7715e1a
Gerrit-Change-Number: 9338
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 15 Feb 2018 22:06:31 +
Gerrit-HasComments: No

[native-toolchain-CR] Bump Kudu version to 0eef8e0

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9338 )

Change subject: Bump Kudu version to 0eef8e0
..

Bump Kudu version to 0eef8e0

This pulls in support for DECIMAL.

Change-Id: I186317c3440e1a50fffa87e67b23166da7715e1a
---
M buildall.sh
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/38/9338/2
--
To view, visit http://gerrit.cloudera.org:8080/9338
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I186317c3440e1a50fffa87e67b23166da7715e1a
Gerrit-Change-Number: 9338
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 

[native-toolchain-CR] Bump Kudu version to 0eef8e0

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9338


Change subject: Bump Kudu version to 0eef8e0
..

Bump Kudu version to 0eef8e0

Change-Id: I186317c3440e1a50fffa87e67b23166da7715e1a
---
M buildall.sh
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/38/9338/1
--
To view, visit http://gerrit.cloudera.org:8080/9338
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I186317c3440e1a50fffa87e67b23166da7715e1a
Gerrit-Change-Number: 9338
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9291 )

Change subject: IMPALA-6508: add KRPC test flag
..


Patch Set 9:

(3 comments)

Thanks for the reviews, please see PS9.

http://gerrit.cloudera.org:8080/#/c/9291/5/tests/common/custom_cluster_test_suite.py
File tests/common/custom_cluster_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/9291/5/tests/common/custom_cluster_test_suite.py@137
PS5, Line 137: if use_exclusive_coordinators:
 :   cmd.append("--use_exclusive_coordinato
> Not sure I'm totally following (haven't really looked much at the custom cl
Thanks for the feedback here and on the dev@ list. I added a comment to clarify 
that we ignore TEST_START_CLUSTER_ARGS on purpose here.


http://gerrit.cloudera.org:8080/#/c/9291/8/tests/custom_cluster/test_exchange_delays.py
File tests/custom_cluster/test_exchange_delays.py:

http://gerrit.cloudera.org:8080/#/c/9291/8/tests/custom_cluster/test_exchange_delays.py@31
PS8, Line 31: @SkipIfBuildType.n
> May want to remove it now that the fix for IMPALA-6512 is being merged.
Done


http://gerrit.cloudera.org:8080/#/c/9291/8/tests/custom_cluster/test_krpc_mem_usage.py
File tests/custom_cluster/test_krpc_mem_usage.py:

http://gerrit.cloudera.org:8080/#/c/9291/8/tests/custom_cluster/test_krpc_mem_usage.py@64
PS8, Line 64:
> If this applies to all the tests here (it appears to), then I think SkipIf
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 9
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 22:05:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[Lars Volker (Code Review)]

Hello Michael Ho, Michael Brown, Sailesh Mukil, David Knupp,

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

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

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

Change subject: IMPALA-6508: add KRPC test flag
..

IMPALA-6508: add KRPC test flag

This change adds a flag "--use_krpc" to start-impala-cluster.py. The
flag is currently passed as an argument to the impalad daemon. In the
future it will also enable KRPC for the catalogd and statestored
daemons.

This change also adds a flag "--test_krpc" to pytest. When running tests
using "impala-py.test --test_krpc", the test cluster will be started
by passing "--use_krpc" to start-impala-cluster.py (see above).

This change also adds a SkipIf to skip tests based on whether the
cluster was started with KRPC support or not.

- SkipIf.not_krpc can be used to mark a test that depends on KRPC.
- SkipIf.not_thrift can be used to mark a test that depends on Thrift
  RPC.

This change adds a meta test to make sure that the new SkipIf decorators
work correctly. The test should be removed as soon as real tests have
been added with the new decorators.

Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
---
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
A tests/common/test_skip.py
M tests/conftest.py
M tests/custom_cluster/test_krpc_mem_usage.py
M tests/custom_cluster/test_rpc_exception.py
8 files changed, 84 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/9291/9
--
To view, visit http://gerrit.cloudera.org:8080/9291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 9
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4874: Increase maximum KRPC message size

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9337 )

Change subject: IMPALA-4874: Increase maximum KRPC message size
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9337/1/be/src/kudu/rpc/transfer.cc@38
PS1, Line 38: INT_MAX
How close did you get to this in tests? Are we confident that values very close 
to INT_MAX would still work, i.e. there's no hidden constant overhead that will 
exceed some limit?


http://gerrit.cloudera.org:8080/#/c/9337/1/testdata/workloads/functional-query/queries/QueryTest/large_strings.test
File testdata/workloads/functional-query/queries/QueryTest/large_strings.test:

http://gerrit.cloudera.org:8080/#/c/9337/1/testdata/workloads/functional-query/queries/QueryTest/large_strings.test@220
PS1, Line 220: select cast(fnv_hash(l_comment) as string) as h from 
tpch_parquet.lineitem union all
Did you check how well these compress? Each hash seems to occur 5 times which 
could allow for up to 80% size reduction. Can you salt the comments with some 
random values before hashing them, e.g. fnv_hash(concat(l_comment), "a")) ... 
union ... fnv_hash(concat(l_comment, "b")), ...



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I876bba0536e1d85e41eacd9c0aeccfe5c2126e58
Gerrit-Change-Number: 9337
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 22:01:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9053 )

Change subject: IMPALA-6416: extend Thread::Create to track instance id
..


Patch Set 8: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187
Gerrit-Change-Number: 9053
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 15 Feb 2018 21:58:05 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9053 )

Change subject: IMPALA-6416: extend Thread::Create to track instance id
..

IMPALA-6416: extend Thread::Create to track instance id

This commit builds upon IMPALA-3703. Each thread that
was created through Thread::Create() has a ThreadDebugInfo
object on the stack frame of Thread::SuperviseThread().
This object has stack allocated char buffers that can be
read during a debug session even if we only have minidumps.

However, with the old solution ThreadDebugInfo::instance_id
was set manually for each thread. It is too easy to forget
to set instance_id every time we create a new thread.

This commit has the assumption that if a thread has an
instance id associated, then the threads spawned by it will
always work on the same instance id. In Thread::StartThread
the parent thread passes its ThreadDebugInfo object to
its child who copies the instance id, and also stores the
name and system thread id of its parent.

This means if we set ThreadDebugInfo::instance_id in some
"root thread", then all descendant threads will annotate
themselves with the instance id automatically. Since threads
also record the name (and a system thread id) of their parent,
it might be also possible to reconstruct the thread creation
graph.

With GDB I tested if it copies the instance id at every
place where we previously needed to set it manually.

I added an automated test to thread-debug-info-test.cc

Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187
Reviewed-on: http://gerrit.cloudera.org:8080/9053
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M be/src/common/thread-debug-info-test.cc
M be/src/common/thread-debug-info.cc
M be/src/common/thread-debug-info.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/util/thread.cc
M be/src/util/thread.h
8 files changed, 75 insertions(+), 25 deletions(-)

Approvals:
  Dan Hecht: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187
Gerrit-Change-Number: 9053
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh
File bin/mvn-quiet.sh:

http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@35
PS5, Line 35: mvn "$@"
Regarding the pipe to mvn - my bad...I though I had removed it in the previous 
match I was trying to move around things to see the output and forgot to 
remove this.
Fixed the rest of the comments. Will push the changes soon.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 5
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 21:54:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4874: Increase maximum KRPC message size

[Joe McDonnell (Code Review)]

Joe McDonnell has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9337


Change subject: IMPALA-4874: Increase maximum KRPC message size
..

IMPALA-4874: Increase maximum KRPC message size

The default value for rpc_max_message_size is 50MB.
Impala currently requires support for messages of
up to 2GB. This changes the value of rpc_max_message_size
to INT_MAX for Impala.

Testing:
- Added a test to test_very_large_strings that generates
  a row with multiple large strings. This row requires
  that the RPC framework successfully transmit over
  300MB. This works for both KRPC and Thrift.
  This query operates under the same amount of memory
  as other queries in large_strings.test.
- Tested separately that larger row sizes also work,
  including tests above 600MB.

Change-Id: I876bba0536e1d85e41eacd9c0aeccfe5c2126e58
---
M be/src/kudu/rpc/transfer.cc
M testdata/workloads/functional-query/queries/QueryTest/large_strings.test
2 files changed, 20 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I876bba0536e1d85e41eacd9c0aeccfe5c2126e58
Gerrit-Change-Number: 9337
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh
File bin/mvn-quiet.sh:

http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@25
PS5, Line 25: $IMPALA_MVN_LOGS_DIR
> This should be quoted in case a parent directory has a space.
Sure...will do


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@27
PS5, Line 27: #Print the output of mvn commands to a dedicated log file in 
$IMPALA_HOME/logs/mvn/mvn.log
> I'm not sure this comment is needed. Typically comments should say why is s
ok


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@29
PS5, Line 29: echo -e " 
\
:  \n Running mvn $IMPALA_MAVEN_OPTIONS $@ \
:  \n Directory: $(pwd) \
:  \n 
" >> 
$LOG_FILE
> I find this to be a little cluttered, with the backslashes and the \n at th
Sure...will do


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@35
PS5, Line 35: mvn "$@" | tee -a $LOG_FILE | grep -E -e WARNING -e ERROR -e 
SUCCESS -e FAILURE -e Test; then
> > * I still think mvn being piped to mvn is an odd construction, and probab
Working on the review comments



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 5
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 19:28:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
14 files changed, 83 insertions(+), 175 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Fredy Wijaya (Code Review)]

Fredy Wijaya has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..

IMPALA-6337: Fix infinite loop in Impala shell

The patch uses a workaround for a bug in the sqlparse.split() function
by joining the statements that contain error tokens into a single
statement

Testing:
- Ran end-to-end shell tests

Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
---
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
2 files changed, 59 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Fredy Wijaya (Code Review)]

Fredy Wijaya has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9195/3/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9195/3/shell/impala_shell.py@579
PS3, Line 579:
> this is returned only when errors come in pairs. otherwise, the joined_stat
Done


http://gerrit.cloudera.org:8080/#/c/9195/3/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/9195/3/tests/shell/test_shell_interactive.py@482
PS3, Line 482: # IMPALA-6337: Fix infinite loop.
> start off with simplest first, perhaps include the simpler repro from the j
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Feb 2018 19:14:29 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Fredy Wijaya (Code Review)]

Fredy Wijaya has uploaded a new patch set (#7). ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..

IMPALA-6337: Fix infinite loop in Impala shell

The patch uses a workaround for a bug in the sqlparse.split() function
by joining the statements that contain error tokens into a single
statement

Testing:
- Ran end-to-end shell tests

Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
---
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
2 files changed, 59 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-5152: Introduce metadata loading phase

[Alex Behm (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Vuk 
Ercegovac,

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

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

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

Change subject: IMPALA-5152: Introduce metadata loading phase
..

IMPALA-5152: Introduce metadata loading phase

Reworks the collection and loading of missing metadata
when compiling a statement. Introduces a new
metadata-loading phase between parsing and analysis.
Summary of the new compilation flow:
1. Parse statement.
2. Collect all table references from the parsed
   statement and generate a list of tables that need
   to be loaded for analysis to succeed.
3. Request missing metadata and wait for it to arrive.
   As views become loaded we expand the set of required
   tables based on the view definitions.
   This step populates a statement-local table cache
   that contains all loaded tables relevant to the
   statement.
4. Create a new Analyzer with the table cache and
   analyze the statement. During analysis only the
   table cache is consulted for table metadata, the
   ImpaladCatalog is not used for that purpose anymore.
5. Authorize the statement.
6. Plan generation as usual.

The intent of the existing code was to collect all tables
missing metadata during analysis, load the metadata, and then
re-analyze the statement (and repeat those steps until all
metadata is loaded).
Unfortunately, the relevant code was hard-to-follow, subtle
and not well tested, and therefore it was broken in several
ways over the course of time. For example, the introduction
of path analysis for nested types subtly broke the intended
behavior, and there are other similar examples.

The serial table loading observed in the JIRA was caused by the
following code in the resolution of table references:
for (all path interpretations) {
  try {
// Try to resolve the path; might call getTable() which
// throws for nonexistent tables.
  } catch (AnalysisException e) {
if (analyzer.hasMissingTbls()) throw e;
  }
}

The following example illustrates the problem:
SELECT * FROM a.b, x.y
When resolving the path "a.b" we consider that "a" could be a
database or a table. Similarly, "b" could be a table or a
nested collection.
If the path resolution for "a.b" adds a missing table entry,
then the path resolution for "x.y" could exit prematurely,
without trying the other path interpretations that would
lead to adding the expected missing table. So effectively,
the tables end up being loaded one-by-one.

Testing:
- A core/hdfs run succeeded
- No new tests were added because the existing functional tests
  provide good coverage of various metadata loading scenarios.
- The issue reported in IMPALA-5152 is basically impossible now.
  Adding FE unit tests for that bug specifically would require
  ugly changes to the new code to enable such testing.

Change-Id: I68d32d5acd4a6f6bc6cedb05e6cc5cf604d24a55
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LimitElement.java
M fe/src/main/java/org/apache/impala/analysis/LoadDataStmt.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/Path.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SetStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateFunctionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M 

[Impala-ASF-CR] IMPALA-5152: Introduce metadata loading phase

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8958 )

Change subject: IMPALA-5152: Introduce metadata loading phase
..


Patch Set 3:

(22 comments)

I did not rebase for now to make reviewing easier.

Still need to rebase and run the full test suite. Should be ok to review the 
big changes.

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

http://gerrit.cloudera.org:8080/#/c/8958/3//COMMIT_MSG@62
PS3, Line 62: Testing:
: - A core/hdfs run succeeded
: - No new tests were added because the existing functional tests
:   provide good coverage of various metadata loading scenarios.
: - The issue reported in IMPALA-5152 is basically impossible now.
:   Adding FE unit tests for that bug specifically would require
:   ugly changes to the new code to enable such testing.
> Your view of these tests is obviously much deeper than mine. We expect that
* Reworked the loading code into a new class, StmtMetadataLoader
* Added FE unit test for the StmtMetadataLoader
* The tests cover basic and interesting cases, but are not exhaustive (I don't 
think that's required or useful)


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@460
PS3, Line 460: // Process statements for which column-level privilege requests 
may be registered
 : // except for DESCRIBE TABLE, REFRESH/INVALIDATE, USE or 
SHOW TABLES statem
> stale comment?
Done


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@462
PS3, Line 462: isResult_.isQueryStmt() || analysisResult_.isInsertStmt() ||
 : analysisResult_.isUpdateStmt() || 
analysisResult_.isDeleteStmt() ||
 : analysi
> worth grouping into a hasColumnPrivilege method?
Done


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@508
PS3, Line 508: sult_.isDescribeTableStmt() ||
 : analysisResult_.isResetMetadataStmt() ||
 : analysisResult_.isUseStmt() ||
 : analysisResult_.isShowTablesStmt() ||
 : analysisResult_
> what's special about these? how would one know that altertable belongs here
Added comment.


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@309
PS3, Line 309: // the map was populated.
> is that TODO about other catalog objects still relevant?
I think so, but I don't think a TODO here in the code will make us more likely 
to address it.


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/LimitElement.java
File fe/src/main/java/org/apache/impala/analysis/LimitElement.java:

http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/LimitElement.java@124
PS3, Line 124: || expr.contains(Subquery.cl
> comment is stale, pls update.
Done


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/Path.java
File fe/src/main/java/org/apache/impala/analysis/Path.java:

http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/Path.java@284
PS3, Line 284:   public static List getCandidateTables(List 
path, String sessionDb) {
> This seems easy to unit test. Would it make sense to do so?
There are ~800 lines of test code dedicated to path analysis
in AnalyzeStmts.java. Those tests exercise this function through the metadata 
loading code path and through the analysis code path.


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java:

http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@62
PS3, Line 62: tableName_.toPath()
> add a precondition in the constructor to check that this is not null.
Done


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java:

http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@71
PS3, Line 71: tableName_.toPath()
> check not-null in constructor.
Done


http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
File 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Michael Brown (Code Review)]

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

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..


Patch Set 5:

(4 comments)

> Line 38: I have tried to make the code more readable now

Please use the inline commenting system to reply to comments left inline. 
Please reply using the "Done" button if you've finished them, or reply inline 
with a response otherwise.

http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh
File bin/mvn-quiet.sh:

http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@25
PS5, Line 25: $IMPALA_MVN_LOGS_DIR
This should be quoted in case a parent directory has a space.


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@27
PS5, Line 27: #Print the output of mvn commands to a dedicated log file in 
$IMPALA_HOME/logs/mvn/mvn.log
I'm not sure this comment is needed. Typically comments should say why is 
something is done, not restate what is already happening in code.


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@29
PS5, Line 29: echo -e " 
\
:  \n Running mvn $IMPALA_MAVEN_OPTIONS $@ \
:  \n Directory: $(pwd) \
:  \n 
" >> 
$LOG_FILE
I find this to be a little cluttered, with the backslashes and the \n at the 
beginning of each line. We also lose the previous verbosity we had in master, 
in which the console showed what how and from where mvn was being run.

How about something like:

  cat << EOF | tee -a "$LOG_FILE"
  ===
  Running mvn $IMPALA_MAVEN_OPTIONS $@
  Directory $(pwd)
  ===
  EOF

This
1. Prints to the console
2. Saves the command context to the log
3. Reduces clutter of backslashes and \n chars.


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@35
PS5, Line 35: mvn "$@" | tee -a $LOG_FILE | grep -E -e WARNING -e ERROR -e 
SUCCESS -e FAILURE -e Test; then
> * I still think mvn being piped to mvn is an odd construction, and probably 
> something to be avoided

Agreed. I don't see how that is necessary here.

 > * I strongly suspect that inside the if-block, as it stands, "echo [...] 
 > exited with code $?" will *always* show an exit code of 0, regardless of 
 > whether mvn failed or not.

Inside the if block, $? will be 0 if mvn has failed. This is because pipefail 
is on, and because the condition: "! mvn ..." is 0. (The ! here is key. It 
negates the 1 returned when mvn fails). You can even use ! outside an if block:

  $ ! false
  $ echo $?
  0
  $

Look at it like this (from master):

1. mvn fails with mvn | grep => ${PIPESTATUS[@]} is 1 0. Overall status of this 
pipe is 1 since pipefail is set.
2. ! negates the 1
3. The if expression evaluation has completed, and $? is set to 0 for the next 
shell command to inspect.
4. If block is entered and $? is still 0 when echo runs.

So you are half right: If we enter that block, $? will be 0, and that echo 
error message is misleading and wrong. But if mvn succeeds, and the rest of the 
pipeline succeeds, the if block will not be entered. $? would have been 
non-zero.

> * That said, I do think that "exit 1" will execute if mvn failed, so at least 
> the script will fail as expected

Agreed.

> * we don't want to see the verbose output of mvn, but we want to capture it 
> in a log file

Agreed.

> * however, if there are lines ERRORS or WARNINGS or SUCCESS or FAILURE, we 
> want to send those lines to the console

Agreed.

> * in the chain of mvn | tee | grep, we want to exit with a non-zero status if 
> mvn fails, but we don't esepcially care about the exit status of tee and 
> certainly not grep

We care less the exit status of mvn, really, we just need to know that it 
failed and exit with failure. Returning mvn's precise exit status would be a 
bonus, I guess.

> I think that all of those things can be accomplished with something like this.
>
>   set -uo pipefail  # i.e., get rid of set -e
>
>   [...]
>
>   mvn $IMPALA_MAVEN_OPTIONS "$@" | tee -a $LOG_FILE | \
>   grep -E -e WARNING -e ERROR -e SUCCESS -e FAILURE -e Test
>
>   MVN_EXIT_STATUS=${PIPESTATUS[0]}
>   echo "mvn exited with ${MVN_EXIT_STATUS}"
>   exit ${MVN_EXIT_STATUS}

I think your solution would work. In particular, the key expression is:

>   mvn $IMPALA_MAVEN_OPTIONS "$@" | tee -a $LOG_FILE | \
>   grep -E -e WARNING -e ERROR -e SUCCESS -e FAILURE -e Test

This sort of expression is what I expected Nithya to come up with on this 
patch, but directly in the if expression, and preserving !.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 

[Impala-ASF-CR] IMPALA-6515: [docs] HAproxy with sticky session requires the check option

[Alex Rodoni (Code Review)]

Alex Rodoni has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9293


Change subject: IMPALA-6515: [docs] HAproxy with sticky session requires the 
check option
..

IMPALA-6515: [docs] HAproxy with sticky session requires the check option

Change-Id: I47165df6624f958c2e0542e2627d4f5377789ab8
---
M docs/topics/impala_proxy.xml
1 file changed, 10 insertions(+), 4 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I47165df6624f958c2e0542e2627d4f5377789ab8
Gerrit-Change-Number: 9293
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4835: Part 2: Allocate scan range buffers upfront

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8707 )

Change subject: IMPALA-4835: Part 2: Allocate scan range buffers upfront
..


Patch Set 25:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8707/25/be/src/runtime/io/request-context.cc
File be/src/runtime/io/request-context.cc:

http://gerrit.cloudera.org:8080/#/c/8707/25/be/src/runtime/io/request-context.cc@154
PS25, Line 154: schedule_mode
> having these three cases is a bit confusing, note to self to think about th
maybe updating the " A scan range for the reader is on one of five states:" 
comment in the header file to include these new states would help.  Also being 
more precise about what we mean by "schedule" and "disk queue" in the header 
comment for this function would help.


http://gerrit.cloudera.org:8080/#/c/8707/25/be/src/runtime/io/scan-range.cc
File be/src/runtime/io/scan-range.cc:

http://gerrit.cloudera.org:8080/#/c/8707/25/be/src/runtime/io/scan-range.cc@116
PS25, Line 116:   bool schedule_range = !returned;
rather than having 'returned', could we just start the scan range with 
blocked_on_buffer_ == true for NO_BUFFER case?  It seems natural to just treat 
the initial case as being blocked on buffer.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia243bf8b62feeea602b122e0503ea4ba7d3ee70f
Gerrit-Change-Number: 8707
Gerrit-PatchSet: 25
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Feb 2018 18:33:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Vuk Ercegovac (Code Review)]

Vuk Ercegovac has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9195/3/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9195/3/shell/impala_shell.py@579
PS3, Line 579: joined_statement
this is returned only when errors come in pairs. otherwise, the 
joined_statement will be dropped. pls clarify the assumptions about what 
parsed_statements contain.


http://gerrit.cloudera.org:8080/#/c/9195/3/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/9195/3/tests/shell/test_shell_interactive.py@482
PS3, Line 482: # IMPALA-6337: Fix infinite loop.
start off with simplest first, perhaps include the simpler repro from the jira.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Feb 2018 18:13:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9053 )

Change subject: IMPALA-6416: extend Thread::Create to track instance id
..


Patch Set 8: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187
Gerrit-Change-Number: 9053
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 15 Feb 2018 18:14:45 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9053 )

Change subject: IMPALA-6416: extend Thread::Create to track instance id
..


Patch Set 7: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187
Gerrit-Change-Number: 9053
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 15 Feb 2018 18:14:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[David Knupp (Code Review)]

David Knupp has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9291 )

Change subject: IMPALA-6508: add KRPC test flag
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9291/5/tests/common/custom_cluster_test_suite.py
File tests/common/custom_cluster_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/9291/5/tests/common/custom_cluster_test_suite.py@137
PS5, Line 137: if pytest.config.option.test_krpc:
 :   cmd.append("--use_krpc")
> I wonder if that's on purpose, would you like me to ask on dev@?
Not sure I'm totally following (haven't really looked much at the custom 
cluster tests), but for what it's worth, I think that it's best that tests be 
100% repeatable, and not vary according to differences between one dev 
environment and another. If we're specifically testing that certain custom args 
work, then we should (I think) exclude TEST_START_CLUSTER_ARGS.


http://gerrit.cloudera.org:8080/#/c/9291/8/tests/custom_cluster/test_krpc_mem_usage.py
File tests/custom_cluster/test_krpc_mem_usage.py:

http://gerrit.cloudera.org:8080/#/c/9291/8/tests/custom_cluster/test_krpc_mem_usage.py@64
PS8, Line 64:   @SkipIf.not_krpc
If this applies to all the tests here (it appears to), then I think SkipIf 
decorator can be applied to the entire test class.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 17:32:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6295: Fix mix/max handling of 'nan' and 'inf'

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8854 )

Change subject: IMPALA-6295: Fix mix/max handling of 'nan' and 'inf'
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8854/4/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8854/4/be/src/exprs/aggregate-functions-ir.cc@611
PS4, Line 611: src.val != src.val
In light of IMPALA-6527 I revisited this code. Is it possible for the compiler 
to optimize this check away? Should we use std::isnan() instead?

http://en.cppreference.com/w/cpp/numeric/math/isnan



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1e206105937ce5afc75ca5044597d39b3dc6a81
Gerrit-Change-Number: 8854
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Feb 2018 17:09:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Fredy Wijaya (Code Review)]

Fredy Wijaya has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..

IMPALA-6337: Fix infinite loop in Impala shell

The patch uses a workaround for a bug in the sqlparse.split() function
by joining the statements that contain error tokens into a single
statement

Testing:
- Ran end-to-end shell tests

Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
---
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
2 files changed, 42 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..


Patch Set 5:

> Patch Set 3:
>
> (2 comments)Line 26 :  I have addressed this comment by keeping the exports 
> in impala-config.sh

Line 38: I have tried to make the code more readable now


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 5
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 14:47:51 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..


Patch Set 5:

> Patch Set 1:
>
> I think this is a good start, but this patch be improved again. I concede too 
> that the IMPALA-5139 description doesn't provide full context. Let me provide 
> some now:
>
> - I think mvn-quiet.sh capturing of mvn INFO output should go into one or 
> more files rooted in $IMPALA_LOG_DIR. This will let Jenkins jobs which tend 
> to look in that place collect the mvn logs, too.
>
> - The working directory and args matter. You can see that's already captured 
> L25-26 of this file, but I think we need that info alongside all the maven 
> output and be written to the log.
>
> - More than one thing calls mvn-quiet.sh. You need a strategy for appending 
> to a log file or writing multiple log files. tee -a might be of assistance 
> here.

Adding my responses to the review comments long after I have addressed them,

- Thanks Michael. I have addressed your comments to
  --- Write the mvn outputs to a single log file
  --- log file located in $IMPALA_HOME/logs/mvn


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 5
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 14:45:51 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id

[Zoltan Borok-Nagy (Code Review)]

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9053 )

Change subject: IMPALA-6416: extend Thread::Create to track instance id
..


Patch Set 7:

Fixed the clang-tidy warnings.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187
Gerrit-Change-Number: 9053
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 15 Feb 2018 13:28:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id

[Zoltan Borok-Nagy (Code Review)]

Hello Lars Volker, Tim Armstrong, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6416: extend Thread::Create to track instance id
..

IMPALA-6416: extend Thread::Create to track instance id

This commit builds upon IMPALA-3703. Each thread that
was created through Thread::Create() has a ThreadDebugInfo
object on the stack frame of Thread::SuperviseThread().
This object has stack allocated char buffers that can be
read during a debug session even if we only have minidumps.

However, with the old solution ThreadDebugInfo::instance_id
was set manually for each thread. It is too easy to forget
to set instance_id every time we create a new thread.

This commit has the assumption that if a thread has an
instance id associated, then the threads spawned by it will
always work on the same instance id. In Thread::StartThread
the parent thread passes its ThreadDebugInfo object to
its child who copies the instance id, and also stores the
name and system thread id of its parent.

This means if we set ThreadDebugInfo::instance_id in some
"root thread", then all descendant threads will annotate
themselves with the instance id automatically. Since threads
also record the name (and a system thread id) of their parent,
it might be also possible to reconstruct the thread creation
graph.

With GDB I tested if it copies the instance id at every
place where we previously needed to set it manually.

I added an automated test to thread-debug-info-test.cc

Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187
---
M be/src/common/thread-debug-info-test.cc
M be/src/common/thread-debug-info.cc
M be/src/common/thread-debug-info.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/util/thread.cc
M be/src/util/thread.h
8 files changed, 75 insertions(+), 25 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187
Gerrit-Change-Number: 9053
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-6512: Fix test exchange delays for KRPC

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9331 )

Change subject: IMPALA-6512: Fix test_exchange_delays for KRPC
..

IMPALA-6512: Fix test_exchange_delays for KRPC

The sender timed out error message diverges between Thrift and KRPC
slightly due to the source address being not readily available with
Thrift RPC implementation. This leads to failure in test_exchange_delays
when KRPC is enabled.

This change fixes the problem by shortening the error message string
to match against.

Testing done: Tested with KRPC enabled in the code and verified the tests 
passed.

Change-Id: Idd9410381dbb931231c92f084917265e5067b4c9
Reviewed-on: http://gerrit.cloudera.org:8080/9331
Reviewed-by: Sailesh Mukil 
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M 
testdata/workloads/functional-query/queries/QueryTest/exchange-delays-zero-rows.test
M testdata/workloads/functional-query/queries/QueryTest/exchange-delays.test
2 files changed, 2 insertions(+), 2 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, but someone else must approve
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idd9410381dbb931231c92f084917265e5067b4c9
Gerrit-Change-Number: 9331
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6512: Fix test exchange delays for KRPC

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9331 )

Change subject: IMPALA-6512: Fix test_exchange_delays for KRPC
..


Patch Set 1: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd9410381dbb931231c92f084917265e5067b4c9
Gerrit-Change-Number: 9331
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Feb 2018 10:55:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[Michael Ho (Code Review)]

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

Change subject: IMPALA-6508: add KRPC test flag
..


Patch Set 8:

Michael and/or David may want to do another pass and +2 the change.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 08:57:51 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6508: add KRPC test flag

[Michael Ho (Code Review)]

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

Change subject: IMPALA-6508: add KRPC test flag
..


Patch Set 8: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9291/8/tests/custom_cluster/test_exchange_delays.py
File tests/custom_cluster/test_exchange_delays.py:

http://gerrit.cloudera.org:8080/#/c/9291/8/tests/custom_cluster/test_exchange_delays.py@31
PS8, Line 31: @SkipIf.not_thrift
May want to remove it now that the fix for IMPALA-6512 is being merged.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Feb 2018 08:56:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

[Michael Ho (Code Review)]

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service 
queue
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h
File be/src/rpc/rpc-mgr-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h@133
PS1, Line 133:   // Takes over ownership of the newly created 'service'.
> Shouldn't the RpcMgr own the services then?
It used to be the case that ImpalaServicePool owns the service before this 
change. I intentionally factored that out and transferred the ownership to 
ExeEnv. DataStreamService is a global singleton. I found it useful to be able 
to access DataStreamService (via ExecEnv) for extended functionality (e.g. per 
thread cache of some objects) in the past during prototyping. Will update the 
comment in DataStreamService to indicate that it should be singleton.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Feb 2018 08:07:40 +
Gerrit-HasComments: Yes