date:20170803

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..


Patch Set 1:

Previous job failed with flaky data load. I filed IMPALA-5765

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5757: Make tbl property toSql deterministic

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[native-toolchain-CR] Upgrade OpenSSL to 1.0.2l

Michael Ho has posted comments on this change.

Change subject: Upgrade OpenSSL to 1.0.2l
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib83c72cce0e64b24729affb5ba10a1e57bc5f837
Gerrit-PatchSet: 2
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[native-toolchain-CR] Upgrade OpenSSL to 1.0.2l

Michael Ho has submitted this change and it was merged.

Change subject: Upgrade OpenSSL to 1.0.2l
..


Upgrade OpenSSL to 1.0.2l

1.0.2l contains fixes for quite a number of CVEs against
the existing version 1.0.1p in the toolchain.

Change-Id: Ib83c72cce0e64b24729affb5ba10a1e57bc5f837
---
M buildall.sh
1 file changed, 5 insertions(+), 2 deletions(-)

Approvals:
  Michael Ho: Verified
  Henry Robinson: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib83c72cce0e64b24729affb5ba10a1e57bc5f837
Gerrit-PatchSet: 2
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho

[Impala-ASF-CR] IMPALA-5742: De-allocate buffer in parquet-reader on exit

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5742: De-allocate buffer in parquet-reader on exit
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf163f858b42d2ca63a7a65d6e457539de59ab9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5742: De-allocate buffer in parquet-reader on exit

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5742: De-allocate buffer in parquet-reader on exit
..


Patch Set 3:

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/981/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf163f858b42d2ca63a7a65d6e457539de59ab9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[native-toolchain-CR] Upgrade OpenSSL to 1.0.2l

Henry Robinson has posted comments on this change.

Change subject: Upgrade OpenSSL to 1.0.2l
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib83c72cce0e64b24729affb5ba10a1e57bc5f837
Gerrit-PatchSet: 2
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5749: coordinator race hits DCHECK 'num remaining backends > 0'

Michael Ho has posted comments on this change.

Change subject: IMPALA-5749: coordinator race hits DCHECK 
'num_remaining_backends_ > 0'
..


Patch Set 1: Code-Review+1

(1 comment)

Not sure if Henry has more comments ?

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

Line 75:   /// this update was not applied because this backend has previously 
completed.
Please also add:

This can happen because the final update for a fragment instance may arrive 
concurrently with previous updates and they can be processed out of order.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1528661e5df6d9732ebfeb414576c82ec5c92241
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5757: Make tbl property toSql deterministic

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..


Patch Set 1: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/982/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5749: coordinator race hits DCHECK 'num remaining backends > 0'

Michael Ho has posted comments on this change.

Change subject: IMPALA-5749: coordinator race hits DCHECK 
'num_remaining_backends_ > 0'
..


Patch Set 1:

(1 comment)

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

PS1, Line 239:   // If this backend completed previously, don't apply the 
update.
 :   if (IsDoneInternal()) return false;
> Sorry, I don't quite understand the race here. Why won't line 250 below pre
Never mind. I mis-read the commit message. The DHCECK triggered is not line 267 
below. Sorry for the noise.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1528661e5df6d9732ebfeb414576c82ec5c92241
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5749: coordinator race hits DCHECK 'num remaining backends > 0'

Michael Ho has posted comments on this change.

Change subject: IMPALA-5749: coordinator race hits DCHECK 
'num_remaining_backends_ > 0'
..


Patch Set 1:

(1 comment)

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

PS1, Line 239:   // If this backend completed previously, don't apply the 
update.
 :   if (IsDoneInternal()) return false;
Sorry, I don't quite understand the race here. Why won't line 250 below prevent 
the same fragment instance from decrementing num_remaining_instances_ twice ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1528661e5df6d9732ebfeb414576c82ec5c92241
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5546: Allow creating unpartitioned Kudu tables

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5546: Allow creating unpartitioned Kudu tables
..


Patch Set 3: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/980/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I281f173dbec1484eb13434d53ea581a0f245358a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[native-toolchain-CR] Upgrade OpenSSL to 1.0.2l

Michael Ho has uploaded a new patch set (#2).

Change subject: Upgrade OpenSSL to 1.0.2l
..

Upgrade OpenSSL to 1.0.2l

1.0.2l contains fixes for quite a number of CVEs against
the existing version 1.0.1p in the toolchain.

Change-Id: Ib83c72cce0e64b24729affb5ba10a1e57bc5f837
---
M buildall.sh
1 file changed, 5 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib83c72cce0e64b24729affb5ba10a1e57bc5f837
Gerrit-PatchSet: 2
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Michael Ho

[native-toolchain-CR] Upgrade OpenSSL to 1.0.2l

Michael Ho has uploaded a new change for review.

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

Change subject: Upgrade OpenSSL to 1.0.2l
..

Upgrade OpenSSL to 1.0.2l

Change-Id: Ib83c72cce0e64b24729affb5ba10a1e57bc5f837
---
M buildall.sh
1 file changed, 5 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib83c72cce0e64b24729affb5ba10a1e57bc5f837
Gerrit-PatchSet: 1
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Michael Ho

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

2017-08-03 Thread Zach Amsden (Code Review)

Zach Amsden has uploaded a new change for review.

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

Change subject: IMPALA-5764: Allow overriding packaged components
..

IMPALA-5764: Allow overriding packaged components

For allowing multiple different distributions to build against the same
codebase, allow overriding various environment variables as well as what
packages to download into the toolchain.  This allows multiple sets of
packaged components to exist in the toolchain and testdata concurrently,
and to easily swap between them using environment overrides.

Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
---
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M testdata/cluster/admin
3 files changed, 56 insertions(+), 43 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden

[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface

Michael Ho has posted comments on this change.

Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream 
interface
..


Patch Set 6:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/exec/exchange-node.h
File be/src/exec/exchange-node.h:

PS6, Line 39: in
nit: long line


http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/data-stream-mgr-base.h
File be/src/runtime/data-stream-mgr-base.h:

PS6, Line 36: impelementations
typo.


PS6, Line 36: , one for Thrfit and KRPC each
for Thrift and KRPC respectively.


http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/data-stream-recvr-base.h
File be/src/runtime/data-stream-recvr-base.h:

PS6, Line 33: , one for Thrfit and KRPC each
for Thrift and KRPC respectively.


PS6, Line 33: impelementations
implementations


PS6, Line 47: /
extra /


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

PS6, Line 79: KRPC
"Kudu RPC" for clarity.


http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:

PS6, Line 92:   /// We implement these in the .cc file to avoid including 
header files for the derived
:   /// classes in this header file; since the implementations 
require a dynamic cast from
:   /// the base type DataStreamMgrBase to the respective derived 
type and the compiler
:   /// cannot determine the base-derived relationship from just 
forward declares.
IMHO, this doesn't seem to provide much value in explaining it here. Can you 
please consider removing it or putting it in .cc file if you think it's useful 
for documentation.

Please add extra explanation on why most callers should use stream_mgr() 
instead of these interfaces.


http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/krpc-data-stream-mgr.h
File be/src/runtime/krpc-data-stream-mgr.h:

PS6, Line 48:   
nit: blank space


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

PS6, Line 28: Unexpected: A KrpcDataStreamRecvr object should not be "
:   "created if the 'use_krpc' flag is false.
Can be simplified to "Shouldn't reach here unless startup flag 'use_krpc' is 
true.


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

PS6, Line 30: Krpc
nit: KRPC


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..


Patch Set 3: Code-Review+1

(3 comments)

Looks good to me. Lets see if Michael wants to take another look.

http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

PS2, Line 200: Various backend tests do not 
> Lots of backend tests rely on the destructor cleaning these up. It's a pret
Ah right, thanks.


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

PS2, Line 105:   /// Closes the MemTracker and deregisters it from its parent. 
Can be called before
 :   /// destruction to prevent other threads from getting a 
reference to the MemTracker
 :   /// via its par
> I'm not sure if I understood the exact suggestion. I combined this with Clo
Yup works for me, thanks


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

Line 260: void RuntimeState::ReleaseResources() {
> I think it's mostly depended on what was simpler in context - in a lot of p
Yeah, makes sense. I think it'd be easier to reason about if it were consistent 
but that's not really something that would need to be addressed now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Hello Matthew Jacobs,

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

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

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

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..

IMPALA-5715: (mitigation only) defer destruction of MemTrackers

One potential candidate for the bad MemTracker IMPALA-5715 is one owned
by a join build sink. I haven't found a clear root cause, but we can
reduce the probability of bugs like this by deferring teardown of the
MemTrackers.

This patch defers destruction of the fragment instance, ExecNode,
DataSink and Codegen MemTrackers until query teardown. Instead
MemTracker::Close() is called at the place where the MemTracker
would have been destroyed to check that all memory is released
and enforce that no more memory is consumed. The entire query
MemTracker subtree is then disconnected in whole from the global
tree in QueryState::ReleaseResources(), instead of the MemTrackers
being incrementally disconnected bottom-up.

In cases where the MemTracker is owned by another object, this
required deferring teardown of the owner until query teardown.
E.g. for LlvmCodeGen I added a Close() method to release resources
and deferred calling the destructor.

We want to make this change anyway - see IMPALA-5587.

Testing:
Ran a core ASAN build.

Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/exchange-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/runtime/buffered-block-mgr.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
20 files changed, 118 insertions(+), 83 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/buffered-block-mgr.cc
File be/src/runtime/buffered-block-mgr.cc:

PS2, Line 568:   mem_tracker_->UnregisterFromParent();
> I don't think you have to UnregisterFromParent here, since the query-state 
I think fixing this is non-trivial, since we call ~BufferedBlockMgr() from 
RuntimeState::ReleaseResources(), which can happen before query teardown.

Hopefully we're deleting this file in a couple of days anyway :).

I explained that it was only used from the query MemTracker and added a TODO to 
update it.


http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

PS2, Line 200: May not have been closed yet.
> why not?
Lots of backend tests rely on the destructor cleaning these up. It's a pretty 
big task to go through and fix them all. Updated the comment to be more 
specific.


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

PS2, Line 105:   /// Deregisters the MemTracker from it's parent. Can be called 
after Close() and before
 :   /// destruction to prevent other threads from getting a 
reference to the MemTracker via
 :   /// its parent.
> I think we can simplify the protocol further: I think this can be be called
I'm not sure if I understood the exact suggestion. I combined this with Close() 
instead of requiring the two methods be called in sequence.


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

Line 260: void RuntimeState::ReleaseResources() {
> This function doesn't appear to be idempotent right now, looks like calling
I think it's mostly depended on what was simpler in context - in a lot of 
places the code is simpler if we call Close() unconditionally.

This one only has a single callsite so there's no reason to think that we'd 
call it twice.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..


Patch Set 2:

(4 comments)

Maybe I've been staring at this too long, but why do we have ReleaseResources() 
instead of Close() where it's used? It's confusing to track which ones are 
meant to be idempotent and which ones aren't.

http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/buffered-block-mgr.cc
File be/src/runtime/buffered-block-mgr.cc:

PS2, Line 568:   mem_tracker_->UnregisterFromParent();
I don't think you have to UnregisterFromParent here, since the query-state will 
unregister its query tracker shortly after this.

It'd be nice to have just 1 place where we ever call UnregisterFromParent, i.e. 
in QueryState::ReleaseResources. Then we can clarify the policy for calling 
UnregisterFromParent in its fn comment, and DCHECK it's only called for query 
trackers.


http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

PS2, Line 200: May not have been closed yet.
why not?


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

PS2, Line 105:   /// Deregisters the MemTracker from it's parent. Can be called 
after Close() and before
 :   /// destruction to prevent other threads from getting a 
reference to the MemTracker via
 :   /// its parent.
I think we can simplify the protocol further: I think this can be be called 
specifically for the query tracker on QueryState::ReleaseResources.


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

Line 260: void RuntimeState::ReleaseResources() {
This function doesn't appear to be idempotent right now, looks like calling 
UnregisterPool() twice with the same resource_pool_ is invalid. Is there a 
consistent story for what release/close methods should be idempotent and what 
shouldn't be?

I don't see any reason this would be called twice, so can you add a 
DCHECK(!released_resources_) for now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Guide to important environment variables for build, test, and mini-cluster operations.

2017-08-03 Thread Zach Amsden (Code Review)

Zach Amsden has posted comments on this change.

Change subject: Guide to important environment variables for build, test, and 
mini-cluster operations.
..


Patch Set 2:

I cut this back in scope somewhat and made it use README.md so that it shows up 
nicely formatted.  I'll go on and document the mini-cluster separately and file 
a JIRA for this if everyone is happy with the format.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I16d34cb4fa0c60c5ad6d9c8764cc0ec21c5cb368
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 7:

I thought about it some more and I'm not sure that adding meaningful timers is 
simple. We already have TotalStorageWaitTime that measures the time the client 
spent blocked for a fragment. It might make sense to add a timer per exec node 
called "IoWaitTime" or "StorageWaitTime" that gives the granular timing.

It's harder to produce a meaningful time for how long the queue was full, i.e. 
we were blocked on compute, because there could be many scan ranges per thread. 
We could include a counter of the number of times the queue was filled up, e.g. 
"IoBufferQueueFullCount", but that might not be terribly intuitive.

I think that would give us enough information to see if the queue was too small 
- in that case we'd see significant IoWaitTime and non-zero 
IoBufferQueueFullCount

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Guide to important environment variables for build, test, and mini-cluster operations.

2017-08-03 Thread Zach Amsden (Code Review)

Zach Amsden has uploaded a new patch set (#2).

Change subject: Guide to important environment variables for build, test, and 
mini-cluster operations.
..

Guide to important environment variables for
build, test, and mini-cluster operations.

Change-Id: I16d34cb4fa0c60c5ad6d9c8764cc0ec21c5cb368
---
M README.md
M bin/impala-config.sh
2 files changed, 72 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16d34cb4fa0c60c5ad6d9c8764cc0ec21c5cb368
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Zach Amsden

[Impala-ASF-CR] IMPALA-5757: Make tbl property toSql deterministic

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5757: Make tbl property toSql deterministic

2017-08-03 Thread Jim Apple (Code Review)

Jim Apple has posted comments on this change.

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..


Patch Set 1: Code-Review+2

> > > Didn't find any test cases that needed to be updated when I ran
 > > the
 > > > tests I was aware of using show create table. Possibly I missed
 > > > some, but will run a jenkins job.
 > >
 > > I'm surprised: it seems like kudu.master_addresses would sort
 > > before kudu.table_name
 > 
 > Agreed, but that's the order in the expected string:
 > 
 > 
 > props = "'kudu.table_name'='%s'" % kudu_table
 > self.assert_show_create_equals(cursor,
 > """
 > CREATE TABLE {{table}} (c INT PRIMARY KEY)
 > PARTITION BY HASH (c) PARTITIONS 3
 > STORED AS KUDU
 > TBLPROPERTIES ({props})""".format(props=props),
 > """
 > CREATE TABLE {db}.{{table}} (
 > c INT NOT NULL ENCODING AUTO_ENCODING COMPRESSION DEFAULT_COMPRESSION,
 > PRIMARY KEY (c)
 > )
 > PARTITION BY HASH (c) PARTITIONS 3
 > STORED AS KUDU
 > TBLPROPERTIES ('kudu.master_addresses'='{kudu_addr}',
 > {props})""".format(

Ah, I mixed up expected and actual

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5757: Make tbl property toSql deterministic

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..


Patch Set 1:

> > Didn't find any test cases that needed to be updated when I ran
 > the
 > > tests I was aware of using show create table. Possibly I missed
 > > some, but will run a jenkins job.
 > 
 > I'm surprised: it seems like kudu.master_addresses would sort
 > before kudu.table_name

Agreed, but that's the order in the expected string:


props = "'kudu.table_name'='%s'" % kudu_table
self.assert_show_create_equals(cursor,
"""
CREATE TABLE {{table}} (c INT PRIMARY KEY)
PARTITION BY HASH (c) PARTITIONS 3
STORED AS KUDU
TBLPROPERTIES ({props})""".format(props=props),
"""
CREATE TABLE {db}.{{table}} (
  c INT NOT NULL ENCODING AUTO_ENCODING COMPRESSION DEFAULT_COMPRESSION,
  PRIMARY KEY (c)
)
PARTITION BY HASH (c) PARTITIONS 3
STORED AS KUDU
TBLPROPERTIES ('kudu.master_addresses'='{kudu_addr}', 
{props})""".format(

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5757: Make tbl property toSql deterministic

2017-08-03 Thread Jim Apple (Code Review)

Jim Apple has posted comments on this change.

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..


Patch Set 1:

> Didn't find any test cases that needed to be updated when I ran the
 > tests I was aware of using show create table. Possibly I missed
 > some, but will run a jenkins job.

I'm surprised: it seems like kudu.master_addresses would sort before 
kudu.table_name

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5757: Make tbl property toSql deterministic

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..


Patch Set 1:

Didn't find any test cases that needed to be updated when I ran the tests I was 
aware of using show create table. Possibly I missed some, but will run a 
jenkins job.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Matthew Jacobs has abandoned this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Abandoned

created a new commit, forgot to take the old Change-Id. New review:  
https://gerrit.cloudera.org/#/c/7580/

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5757: Make tbl property toSql deterministic

Matthew Jacobs has uploaded a new change for review.

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

Change subject: IMPALA-5757: Make tbl property toSql deterministic
..

IMPALA-5757: Make tbl property toSql deterministic

On Ubuntu 16.04, it seems that table properties may be
returned in a different order from that expected by
TestShowCreateTable in test_kudu.py.

The fix is to change ToSqlUtils to print properties in
a deterministic way.

Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
---
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
1 file changed, 12 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2391e04eb40e398098704b77588ad352d514df7d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs

[Impala-ASF-CR] IMPALA-5725: coalesce() with outer join incorrectly rewritten

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5725: coalesce() with outer join incorrectly rewritten
..


Patch Set 2: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/979/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ca6df949f9d416ab207016236dbcb5886295337
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4939, IMPALA-4939: Decimal V2 multiplication

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4939, IMPALA-4939: Decimal V2 multiplication
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7438/4//COMMIT_MSG
Commit Message:

Line 75: DECIMAL_V2 disabled: 2.65s
> It takes time to convert a number from 128 bits to 256 bits. Also, Boost do
So we go down this path for multiplies that overflow, or are close to 
overflowing?

I'd be less concerned if the multiply itself was 4x slower in isolation, but 
10x end-to-end slowdown implies that the slowdown of the operation in isolation 
is more than 10x, probably a lot more.

It seems like we should be able to do much better than that. Might be worth 
doing some profiling to understand where the cycles are going. This is 
dominating query runtime so much you could probably see what's happening with 
disable_codegen=true and perf top if you run the query in a loop.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Lars Volker has posted comments on this change.

Change subject: IMPALA-2869: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/text-converter.cc
File be/src/exec/text-converter.cc:

PS2, Line 126: TextConverter::CodegenWriteSlot
> oops it should be Impala-2689. I will fix the commit message. 
Yes, that sounds reasonable. Please add () to the function names to follow our 
conventions.


Line 281:"WriteSlot function failed verification");
> oops it should be Impala-2689. I will fix the commit message in the next pa
Following the same convention as elsewhere in the code makes sense to me.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2869: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

PS2, Line 326: NULL
Switch to nullptr


http://gerrit.cloudera.org:8080/#/c/7574/1/be/src/exec/text-converter.cc
File be/src/exec/text-converter.cc:

Line 280: Status("TextConverter::CodegenWriteSlot:codegen'd "
?


http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/text-converter.cc
File be/src/exec/text-converter.cc:

Line 76: //   %1 = bitcast <{ i32, i8 }>* %tuple_arg to i8*
> Why did the IR change?
Same question. I'm guessing that the old IR was just stale, in which case - 
thanks for updating.

There's a lot of stale IR in comments since we don't update all the comments in 
the dependency graph when we change a codegen function (or change the LLVM 
version).


Line 125:   if (is_null_string_fn == NULL) {
This could be a DCHECK. We shouldn't be missing builtin functions.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

> > > > (1 comment)
 > > >
 > > > I recently changed several places in the FE to use
 > LinkedHashMaps
 > > > in order to get deterministic ordering. That included
 > > > ToSqlUtils.java:212, so I'm surprised the order is not
 > > > deterministic. I suspect that file has the code that needs
 > > fixing,
 > > > possibly L427 propertyMapToSql(). Maybe it'd be best to sort
 > the
 > > > properties to be completely independent from the underlying
 > > > storage.
 > >
 > > Yup, that looks correct. I think we can just create a
 > LinkedHashSet
 > > from the entrySet(). Hopefully that won't break too many existing
 > > tests.
 > 
 > At that poing propertyMap is already a LinkedHashMap, so I think
 > the order of the entrySet should be stable (see 
 > https://stackoverflow.com/questions/1190083/does-entryset-in-a-linkedhashmap-also-guarantee-order).
 > 
 > I think msTable.getParameters() returns a Map<>, which means we may
 > have to sort it to get a deterministic order.

Yeah I was just realizing that as well. Thanks

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Lars Volker has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

> > > (1 comment)
 > >
 > > I recently changed several places in the FE to use LinkedHashMaps
 > > in order to get deterministic ordering. That included
 > > ToSqlUtils.java:212, so I'm surprised the order is not
 > > deterministic. I suspect that file has the code that needs
 > fixing,
 > > possibly L427 propertyMapToSql(). Maybe it'd be best to sort the
 > > properties to be completely independent from the underlying
 > > storage.
 > 
 > Yup, that looks correct. I think we can just create a LinkedHashSet
 > from the entrySet(). Hopefully that won't break too many existing
 > tests.

At that poing propertyMap is already a LinkedHashMap, so I think the order of 
the entrySet should be stable (see 
https://stackoverflow.com/questions/1190083/does-entryset-in-a-linkedhashmap-also-guarantee-order).

I think msTable.getParameters() returns a Map<>, which means we may have to 
sort it to get a deterministic order.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5742: De-allocate buffer in parquet-reader on exit

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5742: De-allocate buffer in parquet-reader on exit
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf163f858b42d2ca63a7a65d6e457539de59ab9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5572: Timestamp codegen for text scanner

2017-08-03 Thread Tianyi Wang (Code Review)

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5572: Timestamp codegen for text scanner
..


Patch Set 3:

Rebase done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I00cbf8ec7784ca9594e14e952f46dc54a5ede44b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5742: De-allocate buffer in parquet-reader on exit

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5742: De-allocate buffer in parquet-reader on exit
..


Patch Set 3: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/978/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf163f858b42d2ca63a7a65d6e457539de59ab9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5602: Fix kudu queries being incorrectly optimized as small query

2017-08-03 Thread Bikramjeet Vig (Code Review)

Bikramjeet Vig has posted comments on this change.

Change subject: IMPALA-5602: Fix kudu queries being incorrectly optimized as 
small query
..


Patch Set 3:

(1 comment)

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

PS2, Line 802: 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
> This could be confusing because it's not obvious why someone would know to 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I93822d67ebda41d5d0456095c429e3915a3f40c4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5602: Fix kudu queries being incorrectly optimized as small query

2017-08-03 Thread Bikramjeet Vig (Code Review)

Bikramjeet Vig has uploaded a new patch set (#3).

Change subject: IMPALA-5602: Fix kudu queries being incorrectly optimized as 
small query
..

IMPALA-5602: Fix kudu queries being incorrectly optimized as small query

Fix a bug where the following queries on kudu tables were incorrectly
being optimized as a 'small query' and therefore running on a single
node with a single scanner thread:

(A) that have all their predicates pushed to kudu and have a limit
(B) table stats missing + Conditions in (A)

Testing:
For (A): Added a frontend planner test
For (B): Added an end to end test in test_kudu.py

Change-Id: I93822d67ebda41d5d0456095c429e3915a3f40c4
---
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/ScanNode.java
M fe/src/main/java/org/apache/impala/util/MaxRowsProcessedVisitor.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
M tests/query_test/test_kudu.py
6 files changed, 659 insertions(+), 256 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I93822d67ebda41d5d0456095c429e3915a3f40c4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5572: Timestamp codegen for text scanner

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5572: Timestamp codegen for text scanner
..


Patch Set 2: Code-Review+2

Looks great. I can start the merge after you rebase onto the latest master.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I00cbf8ec7784ca9594e14e952f46dc54a5ede44b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
..


Patch Set 1:

(8 comments)

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

Line 276: 
> I would add one more test case where the server is started with a list of c
Done


http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

PS1, Line 166:   friend class ThriftServerBuilder;
> nit: Move just below private: as it's easier to find, plus that's the proto
Done


PS1, Line 247: // Returns true if, per the process configuration flags, 
server<->server communications
 : // should use SSL.
 : bool EnableInternalSslConnections();
> nit: Move below ThriftServerBuilder class or above ThriftServer class.
Done


PS1, Line 254: ThriftServerBuilder(const std::string& name,
 :   const boost::shared_ptr& 
processor, int port)
 : : name_(name), processor_(processor), port_(port) {}
> Builder constructors usually just take the name from what I've seen in the 
The idea is that the c'tors take mandatory parameters. Doing it this way means 
there's no way to construct a server without a name or processor or port, which 
means you don't need to check the error of whether they're set or not. 

I'm not sure what you mean by "options are being set at the caller side vs 
passing members into a constructor" - can you clarify?


PS1, Line 259: auth_provider
> Since this is a builder class, I'd prefer if all these functions were prepe
I prefer not, unless you feel strongly - the 'set_' is redundant with the 
context of it being a builder.


Line 311:   Status Build(ThriftServer** server) {
> Thorough error checking needs to be done on the builder side before creatin
Any use of the server will trigger a failure if it's misconfigured (and I think 
duplicating error checking here probably doesn't make sense for that reason).


http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1947: be_builder.metrics(exec_env->metrics())
> Set metrics just after constructor initialization in L1940 and just call be
Hm, why? That introduces an extra statement, and is kind of against the point 
of a fluent interface (where you can chain things together).


http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

PS1, Line 203: ThriftServer* server;
 : RETURN_IF_ERROR(builder.Build());
 : heartbeat_server_.reset(server);
> Surround with curly braces so that 'server' goes out of scope when it's not
Not sure this is warranted (you could argue the same thing about 'builder'). 
It's usually ok for local variables to extend their scope to the end of a 
method.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5546: Allow creating unpartitioned Kudu tables

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5546: Allow creating unpartitioned Kudu tables
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I281f173dbec1484eb13434d53ea581a0f245358a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift

Henry Robinson has uploaded a new patch set (#2).

Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
..

IMPALA-5696: Enable cipher configuration when using TLS / Thrift

The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allow the cipher
suite to be configured so that ciphers may be removed from the whitelist
if they are shown to be weak.

* Add a flag --ssl_cipher_list which controls cipher selection for both
  thrift servers and clients. Default is blank, which means use all
  available cipher suites.
* Add ThriftServerBuilder to simplify construction of
  ThriftServers (whose constructors were otherwise getting very long).

Testing: new tests added to thrift-server-test. Test cases added follow:

* A client cannot connect to a server which does not have any ciphers in
  common with it.
* If ciphers are identical on clients and servers, that ssl connections
  can be made.
* Bad cipher strings lead to errors on both client and server.

Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
---
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
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/runtime/data-stream-test.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/testutil/in-process-servers.cc
A be/src/testutil/scoped-flag-setter.h
M be/src/util/webserver-test.cc
13 files changed, 426 insertions(+), 144 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

> > (1 comment)
 > 
 > I recently changed several places in the FE to use LinkedHashMaps
 > in order to get deterministic ordering. That included
 > ToSqlUtils.java:212, so I'm surprised the order is not
 > deterministic. I suspect that file has the code that needs fixing,
 > possibly L427 propertyMapToSql(). Maybe it'd be best to sort the
 > properties to be completely independent from the underlying
 > storage.

Yup, that looks correct. I think we can just create a LinkedHashSet from the 
entrySet(). Hopefully that won't break too many existing tests.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5572: Timestamp codegen for text scanner

2017-08-03 Thread Tianyi Wang (Code Review)

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5572: Timestamp codegen for text scanner
..


Patch Set 2:

(6 comments)

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

Line 14: 
> Can you add a short Testing: section here.
Done


http://gerrit.cloudera.org:8080/#/c/7556/1/be/src/exec/hdfs-scanner-ir.cc
File be/src/exec/hdfs-scanner-ir.cc:

Line 119: extern "C"
> It looks like the other functions have extern "C" to avoid mangling the fun
Done


PS1, Line 120: s
> Extra space before *
Done


http://gerrit.cloudera.org:8080/#/c/7556/1/be/src/exec/text-converter.cc
File be/src/exec/text-converter.cc:

PS1, Line 246: 16 byte
> 16 bytes
Done


PS1, Line 268: mov
> nit: move
Done


http://gerrit.cloudera.org:8080/#/c/7556/1/be/src/util/string-parser.h
File be/src/util/string-parser.h:

Line 100:   /// Parse a TimestampValue from s.
> Nit: add a . on the end for consistency with other comments.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I00cbf8ec7784ca9594e14e952f46dc54a5ede44b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Lars Volker has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

> (1 comment)

I recently changed several places in the FE to use LinkedHashMaps in order to 
get deterministic ordering. That included ToSqlUtils.java:212, so I'm surprised 
the order is not deterministic. I suspect that file has the code that needs 
fixing, possibly L427 propertyMapToSql(). Maybe it'd be best to sort the 
properties to be completely independent from the underlying storage.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..


Patch Set 2:

> The conclusion was that it was very difficult to reason about the
 > lifetime of the different MemTrackers and the possible states
 > things could end up in during teardown. The original patch made a
 > small change that was difficult to reason about in isolation.
 > 
 > This doesn't actually address the root cause of IMPALA-5715 - if we
 > figure out what happened there we could probably do a targeted fix.

Yup I agree this makes it much easier to reason about. Thanks.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5546: Allow creating unpartitioned Kudu tables

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5546: Allow creating unpartitioned Kudu tables
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I281f173dbec1484eb13434d53ea581a0f245358a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5749: coordinator race hits DCHECK 'num remaining backends > 0'

2017-08-03 Thread Sailesh Mukil (Code Review)

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5749: coordinator race hits DCHECK 
'num_remaining_backends_ > 0'
..


Patch Set 1:

> Does this trigger only when there are two concurrent calls to
 > UpdateBackendExecStatus() from the same backend? If so, do we
 > understand why that happens so often?

My understanding is this:
A fragment instance sends reports every 'n' seconds. Due to a congested 
network, two of these reports for the same fragment instance from a backend can 
arrive at the coordinator and start being processed at around the same time, 
hence leading to this issue.

Ideally a second report cannot be send until the first one is ACKd by the 
coordinator, since a lock is held until the report is ACKd, in the 
ReportProfileThread(); but there is only one case where a second report will be 
sent before the first one is responded to, i.e.  from 
FragmentInstanceState::Finalize().

So ReportProfileThread() sends the one report of the last finstance, then 
Finalize() sends the second report of the same finstance before the first one 
is responded to.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1528661e5df6d9732ebfeb414576c82ec5c92241
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4939, IMPALA-4939: Decimal V2 multiplication

2017-08-03 Thread Taras Bobrovytsky (Code Review)

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4939, IMPALA-4939: Decimal V2 multiplication
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7438/4//COMMIT_MSG
Commit Message:

Line 75: DECIMAL_V2 disabled: 2.65s
> This is scary and I don't think we should consider merging this until it's 
It takes time to convert a number from 128 bits to 256 bits. Also, Boost does 
something like 4 128 bit multiplications internally in the 256 bit 
multiplication function (so it makes sense for it to be at least 4x slower). I 
think the 256 bit multiplication function is also not optimized for the worst 
case (where the result actually needs more than 128 bits).

Also, it takes time to convert 2 numbers to 256 bit numbers and then convert 1 
number back.

I don't think it's that surprising that it's much slower. 256 bit 
multiplication is slow and is not implemented in hardware like others.

Remember, this benchmark is the absolute worst case scenario, where we do a 256 
bit multiplication for every row.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5749: coordinator race hits DCHECK 'num remaining backends > 0'

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5749: coordinator race hits DCHECK 
'num_remaining_backends_ > 0'
..


Patch Set 1:

Does this trigger only when there are two concurrent calls to 
UpdateBackendExecStatus() from the same backend? If so, do we understand why 
that happens so often?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1528661e5df6d9732ebfeb414576c82ec5c92241
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5749: coordinator race hits DCHECK 'num remaining backends > 0'

2017-08-03 Thread Sailesh Mukil (Code Review)

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5749: coordinator race hits DCHECK 
'num_remaining_backends_ > 0'
..


Patch Set 1: Code-Review+1

(1 comment)

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

PS1, Line 16: so if there are
: two simultaneous calls to UpdateBackendExecStatus()
nit rephrase:
so if there are two reports for the last fragment instance in a backend that 
arrive at the coordinator at the same time, they can call 
UpdateBackendExecStatus() simultaneously, both call IsDone(), ...


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1528661e5df6d9732ebfeb414576c82ec5c92241
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5546: Allow creating unpartitioned Kudu tables

2017-08-03 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-5546: Allow creating unpartitioned Kudu tables
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7446/2/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_create.test:

PS2, Line 273: create table unpartitioned_kudu_table2 primary key(id) stored as 
kudu
 : a
> do you get the warning in this case as well?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I281f173dbec1484eb13434d53ea581a0f245358a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5546: Allow creating unpartitioned Kudu tables

2017-08-03 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has uploaded a new patch set (#3).

Change subject: IMPALA-5546: Allow creating unpartitioned Kudu tables
..

IMPALA-5546: Allow creating unpartitioned Kudu tables

This patch makes it possible to create unpartitioned, managed Kudu
tables from Impala, by making the 'PARTITION BY' clause of 'CREATE
TABLE... STORED AS KUDU' optional:

CREATE TABLE [IF NOT EXISTS] [db_name.]table_name
  (col_name data_type
[kudu_column_attribute ...]
[COMMENT 'col_comment']
[, ...]
[PRIMARY KEY (col_name[, ...])]
  )
  [PARTITION BY kudu_partition_clause]
  [COMMENT 'table_comment']
  STORED AS KUDU
  [TBLPROPERTIES ('key1'='value1', 'key2'='value2', ...)]

Kudu represents this as a table that is range partitioned on no
columns.

Because unpartitioned Kudu tables are inefficient for large data
sizes, and because the syntax doesn't make it explicit that the table
will be unpartitioned, there is a warning issued to encourage users
to created partitioned tables.

This patch also converts the tpch_kudu.nation and tpch_kudu.region
tables to be unpartitioned, as they are very small.

Testing:
- Updated analysis tests.
- Added e2e test that creates unpartitioned table and inserts into it.

Change-Id: I281f173dbec1484eb13434d53ea581a0f245358a
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M testdata/datasets/tpch/tpch_kudu_template.sql
M testdata/datasets/tpch/tpch_schema_template.sql
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
6 files changed, 53 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I281f173dbec1484eb13434d53ea581a0f245358a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall

[Impala-ASF-CR] IMPALA-5749: coordinator race hits DCHECK 'num remaining backends > 0'

2017-08-03 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has uploaded a new change for review.

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

Change subject: IMPALA-5749: coordinator race hits DCHECK 
'num_remaining_backends_ > 0'
..

IMPALA-5749: coordinator race hits DCHECK 'num_remaining_backends_ > 0'

In Coordinator::UpdateBackendExecStatus(), we check if the backend
has already completed with BackendState::IsDone() and return without
applying the update if so to avoid updating num_remaining_backends_
twice for the same completed backend.

The problem is that the value of BackendState::IsDone() is updated by
the call to BackendState::ApplyExecStatusReport() that comes after it,
but these operations are not performed atomically, so if there are
two simultaneous calls to UpdateBackendExecStatus(), they can both
call IsDone(), both get 'false', and then proceed to erroneously both
update num_remaining_backends_, hitting a DCHECK.

The solution is to perform both the call to IsDone() and the update to
it atomically by holding the BackendState::lock_.

Testing:
- Ran test_finst_cancel_when_query_complete 10,000 times without
  hitting the DCHECK (previously, it would hit about once per 300
  runs).

Change-Id: I1528661e5df6d9732ebfeb414576c82ec5c92241
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
3 files changed, 16 insertions(+), 11 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..


Patch Set 2:

The conclusion was that it was very difficult to reason about the lifetime of 
the different MemTrackers and the possible states things could end up in during 
teardown. The original patch made a small change that was difficult to reason 
about in isolation.

This doesn't actually address the root cause of IMPALA-5715 - if we figure out 
what happened there we could probably do a targeted fix.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..


Patch Set 2:

> That was a bit more involved than expected, but all MemTrackers
 > should now have the same lifetime as the QueryState (because they
 > are either in the QueryState object pool or owned by a control
 > structure with that lifetime).

Seems like a good idea, but can you summarize from the offline conversation you 
had why this seemed worth addressing for IMPALA-5715? One thing to keep in mind 
is how easy the patch may be to backport to past releases.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5742: De-allocate buffer in parquet-reader on exit

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5742: De-allocate buffer in parquet-reader on exit
..


Patch Set 3: Code-Review+2

Carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf163f858b42d2ca63a7a65d6e457539de59ab9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3548: Prune runtime filters based on query options in the FE

2017-08-03 Thread Attila Jeges (Code Review)

Attila Jeges has posted comments on this change.

Change subject: IMPALA-3548: Prune runtime filters based on query options in 
the FE
..


Patch Set 2:

(6 comments)

Thanks for the review!

http://gerrit.cloudera.org:8080/#/c/7564/1/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

PS1, Line 119: RuntimeFilterGener
> unnecessary
Done


PS1, Line 121: 
 : 
> single line?
Done


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

Line 538:* The assigned filters are the ones for which 'scanNode' can be 
used a destination
> update this comment
Done


PS1, Line 549: Preconditions.checkNotNull(scanNode);
 : Analyzer analyzer = ctx.getRootAnalyzer();
> single line?
Done


PS1, Line 590: e
> nit: space before ':'
Done


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

Line 1: 
> I suppose these are written as query tests and not planner tests because th
Correct. These test cases should be in the planner test suite, but implementing 
them this way is more straightforward.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3548: Prune runtime filters based on query options in the FE

2017-08-03 Thread Attila Jeges (Code Review)

Attila Jeges has uploaded a new patch set (#2).

Change subject: IMPALA-3548: Prune runtime filters based on query options in 
the FE
..

IMPALA-3548: Prune runtime filters based on query options in the FE

Currently, the FE generates a number of runtime filters and assigns
them to the single node plan without taking the value of
RUNTIME_FILTER_MODE and DISABLE_ROW_RUNTIME_FILTERING query options
into account.

The backend then removes filters from exec nodes, based on the
following rules:
1. If DISABLE_ROW_RUNTIME_FILTERING is set, filters are removed from
   the exec nodes that are marked as targets not bound by partition
   columns.

2. If RUNTIME_FILTER_MODE is set to LOCAL, filters are removed from
   the exec nodes that are marked as remote targets.

This may cause some confusion to users because they may see runtime
filters in the output of explain that are not applied when the query
is executed.

This change moves the logic of runtime filter pruning to the planner
in the FE. The runtime filter assignment is done on the distributed
plan and the above constraints are enforced there directly.

Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb
---
M be/src/exec/hash-join-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/runtime/coordinator.cc
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
A 
testdata/workloads/functional-query/queries/QueryTest/runtime_filters_distrib_pruning.test
A 
testdata/workloads/functional-query/queries/QueryTest/runtime_filters_singlenode_pruning.test
M tests/query_test/test_runtime_filters.py
10 files changed, 666 insertions(+), 93 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

(1 comment)

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

PS1, Line 16: checking for both
> How soon do we need a fix? If this is blocking you or other engineers, we m
I guess you didn't mark this as a blocker so I'll take it to mean this isn't 
needed immediately. I'll do the more general fix when I have a bit more time.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5725: coalesce() with outer join incorrectly rewritten

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5725: coalesce() with outer join incorrectly rewritten
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ca6df949f9d416ab207016236dbcb5886295337
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3548: Prune runtime filters based on query options in the FE

2017-08-03 Thread Attila Jeges (Code Review)

Attila Jeges has uploaded a new patch set (#2).

Change subject: IMPALA-3548: Prune runtime filters based on query options in 
the FE
..

IMPALA-3548: Prune runtime filters based on query options in the FE

Currently, the FE generates a number of runtime filters and assigns
them to the single node plan without taking the value of
RUNTIME_FILTER_MODE and DISABLE_ROW_RUNTIME_FILTERING query options
into account.

The backend then removes filters from exec nodes, based on the
following rules:
1. If DISABLE_ROW_RUNTIME_FILTERING is set, filters are removed from
   the exec nodes that are marked as targets not bound by partition
   columns.

2. If RUNTIME_FILTER_MODE is set to LOCAL, filters are removed from
   the exec nodes that are marked as remote targets.

This may cause some confusion to users because they may see runtime
filters in the output of explain that are not applied when the query
is executed.

This change moves the logic of runtime filter pruning to the planner
in the FE. The runtime filter assignment is done on the distributed
plan and the above constraints are enforced there directly.

Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb
---
M be/src/exec/hash-join-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/runtime/coordinator.cc
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
A 
testdata/workloads/functional-query/queries/QueryTest/runtime_filters_distrib_pruning.test
A 
testdata/workloads/functional-query/queries/QueryTest/runtime_filters_singlenode_pruning.test
M tests/query_test/test_runtime_filters.py
10 files changed, 666 insertions(+), 93 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

(1 comment)

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

PS1, Line 16: checking for both
> I think Lars already did this for some table properties recently.
How soon do we need a fix? If this is blocking you or other engineers, we may 
need to take this and we can do the more general fix later. I don't have 
bandwidth to make the bigger change this week.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

(1 comment)

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

PS1, Line 16: checking for both
> Yeah good point. I assumed it'd be a big change as it could reorder existin
I think Lars already did this for some table properties recently.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

(1 comment)

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

PS1, Line 16: checking for both
> Could we change the table property printing to be deterministic, rather tha
Yeah good point. I assumed it'd be a big change as it could reorder existing 
tests, but I didn't validate that. I'll check this afternoon.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3931: arbitrary fixed-size uda intermediate types

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types
..


Patch Set 7: Code-Review+1

LGTM, thanks. I didn't review the codegen code in detail, deferring to Michael 
for that.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife90cf27989f98ffb5ef5c39f1e09ce92e8cb87c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-03 Thread anujphadke (Code Review)

anujphadke has posted comments on this change.

Change subject: IMPALA-2869: Log why codegen is disabled in
TextConverter::CodegenWriteSlot()
..

Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/text-converter.cc
File be/src/exec/text-converter.cc:

PS2, Line 126: TextConverter::CodegenWriteSlot
> Do you need this part? If the error gets logged, won't it include where it
oops it should be Impala-2689. I will fix the commit message.
This shows up in runtime profile when codegen is disabled.

Ex: from the profile -
ExecOption: TEXT Codegen Disabled: TextConverter::CodegenWriteSlot: Char isn't
supported for CodegenWriteSlot

I think it would be useful to print this.
Let me know what you think? I will fix this in the next patch.

Line 281:"WriteSlot function failed verification");
> Maybe call it "FinalizeFunction() failed"? The error message seems differen
oops it should be Impala-2689. I will fix the commit message in the next patch.

FinalizeFunction returns NULL when verification fails. We have a similar log
message's elsewhere too -
https://github.com/cloudera/Impala/commit/2be7a1723f0340367e050f7cafd8a65fab55e37e#diff-1a1c603752b59b3305d47ba0805f1ca2R597
I ll also append the "check logs" to the message.

Let me know what do you think? I will change it accordingly in the next patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: anujphadke
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..


Patch Set 2:

That was a bit more involved than expected, but all MemTrackers should now have 
the same lifetime as the QueryState (because they are either in the QueryState 
object pool or owned by a control structure with that lifetime).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

Tim Armstrong has uploaded a new patch set (#2).

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..

IMPALA-5715: (mitigation only) defer destruction of MemTrackers

One potential candidate for the bad MemTracker IMPALA-5715 is one owned
by a join build sink. I haven't found a clear root cause, but we can
reduce the probability of bugs like this by deferring teardown of the
MemTrackers.

This patch defers destruction of the fragment instance, ExecNode,
DataSink and Codegen MemTrackers until query teardown. Instead
MemTracker::Close() is called at the place where the MemTracker
would have been destroyed to check that all memory is released
and enforce that no more memory is consumed. The entire query
MemTracker subtree is then disconnected in whole from the global
tree in QueryState::ReleaseResources(), instead of the MemTrackers
being incrementally disconnected bottom-up.

In cases where the MemTracker is owned by another object, this
required deferring teardown of the owner until query teardown.
E.g. for LlvmCodeGen I added a Close() method to release resources
and deferred calling the destructor.

We want to make this change anyway - see IMPALA-5587.

Testing:
TODO

Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/exchange-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/runtime/buffered-block-mgr.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
20 files changed, 114 insertions(+), 79 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5742: De-allocate buffer in parquet-reader on exit

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5742: De-allocate buffer in parquet-reader on exit
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf163f858b42d2ca63a7a65d6e457539de59ab9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5756: start memory maintenance thread after metric creation

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5756: start memory maintenance thread after metric 
creation
..


IMPALA-5756: start memory maintenance thread after metric creation

Daemons were sometimes crashing on startup if the memory maintenance
thread started running before the memory metrics were created.

This changes the startup sequence so that the thread is started
after the metrics are registered.

Change-Id: Ibb7337a68547b4d660f3100b04df2c20befb044c
Reviewed-on: http://gerrit.cloudera.org:8080/7573
Reviewed-by: Henry Robinson 
Tested-by: Impala Public Jenkins
---
M be/src/catalog/catalogd-main.cc
M be/src/common/init.cc
M be/src/common/init.h
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
5 files changed, 13 insertions(+), 8 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Henry Robinson: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibb7337a68547b4d660f3100b04df2c20befb044c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3931: arbitrary fixed-size uda intermediate types

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types
..

Patch Set 6:

(4 comments)

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

PS6, Line 26: FIXED_AGG_INTERMEDIATE
> I think FIXED_UDA_INTERMEDIATE would be more precise/clear, especially sinc
It wasn't too hard with some sed -i.

PS6, Line 37: Testing:
> any corner cases to consider for analytic fns? I think it should work fine,
I couldn't think of any. I think it should work fine too. I agree we don't have
many tests where analytics spill. I added a couple more tests in
https://gerrit.cloudera.org/#/c/7552 but we could probably do with more.

At least the spilling logic in the analytic is straightforward - most of the
complication is in BufferedTupleStream which has a reasonable number of unit
tests.

http://gerrit.cloudera.org:8080/#/c/7526/6/be/src/runtime/types.cc
File be/src/runtime/types.cc:

PS6, Line 172: TypeToOdbcString
> should types that aren't exposed raise a dcheck?
Probably. Changed that in this case because we never will expose it.

http://gerrit.cloudera.org:8080/#/c/7526/6/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
File fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java:

PS6, Line 1000: TODO
> ?
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife90cf27989f98ffb5ef5c39f1e09ce92e8cb87c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3931: arbitrary fixed-size uda intermediate types

Tim Armstrong has uploaded a new patch set (#7).

Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types
..

IMPALA-3931: arbitrary fixed-size uda intermediate types

Make many builtin aggregate functions use fixed-length intermediate
types:
* avg()
* ndv()
* stddev(), variance(), etc
* distinctpc(), distinctpcsa()

sample(), appx_median(), histogram() and group_concat() actually
allocate var-len data so aren't changed.

This has some major benefits:
* Spill-to-disk works properly with these aggregations.
* Aggregations are more efficient because there is one less pointer
  indirection.
* Aggregations use less memory, because we don't need an extra 12-byte
  StringValue for the indirection.

Adds a special-purpose internal type FIXED_UDA_INTERMEDIATE. The type
is represented in the same way as CHAR - a fixed-size array of bytes,
stored inline in tuples. However, it is not user-visible and does
not support CHAR semantics, i.e. users can't declare tables, functions,
etc with the type. The pointer and length is passed into aggregate functions
wrapped in a StringVal.

Updates some internal codegen functions to work better with the new
type. E.g. store values directly into the result tuple instead of
via an intermediate stack allocation.

Testing:
This change only affects builtin aggregate functions, for which we
have test coverage already. If we were to allow wider use of this type,
it would need further testing.

Added an analyzer test to ensure we can't use the type for UDAs.

Added a regression test for spilling avg().

Perf:
Ran TPC-H locally. TPC-H Q17, which has a high-cardinality AVG(),
improved dramatically.

+--+---+-++++
| Workload | File Format   | Avg (s) | Delta(Avg) | GeoMean(s) | 
Delta(GeoMean) |
+--+---+-++++
| TPCH(60) | parquet / none / none | 18.44   | -17.54%| 11.92  | -5.34% 
|
+--+---+-++++

+--+--+---++-++---++-+---+
| Workload | Query| File Format   | Avg(s) | Base Avg(s) | 
Delta(Avg) | StdDev(%) | Base StdDev(%) | Num Clients | Iters |
+--+--+---++-++---++-+---+
| TPCH(60) | TPCH-Q12 | parquet / none / none | 18.40  | 17.64   | +4.32%   
  |   0.77%   |   1.09%| 1   | 5 |
| TPCH(60) | TPCH-Q22 | parquet / none / none | 7.07   | 6.90| +2.36%   
  |   0.28%   |   0.30%| 1   | 5 |
| TPCH(60) | TPCH-Q3  | parquet / none / none | 12.37  | 12.11   | +2.10%   
  |   0.18%   |   0.15%| 1   | 5 |
| TPCH(60) | TPCH-Q7  | parquet / none / none | 42.48  | 42.09   | +0.93%   
  |   2.45%   |   0.80%| 1   | 5 |
| TPCH(60) | TPCH-Q6  | parquet / none / none | 3.18   | 3.15| +0.89%   
  |   0.67%   |   0.76%| 1   | 5 |
| TPCH(60) | TPCH-Q19 | parquet / none / none | 7.24   | 7.20| +0.50%   
  |   0.95%   |   0.67%| 1   | 5 |
| TPCH(60) | TPCH-Q10 | parquet / none / none | 13.37  | 13.30   | +0.50%   
  |   0.48%   |   1.39%| 1   | 5 |
| TPCH(60) | TPCH-Q5  | parquet / none / none | 7.47   | 7.44| +0.36%   
  |   0.58%   |   0.54%| 1   | 5 |
| TPCH(60) | TPCH-Q11 | parquet / none / none | 2.03   | 2.02| +0.06%   
  |   0.26%   |   1.95%| 1   | 5 |
| TPCH(60) | TPCH-Q4  | parquet / none / none | 5.48   | 5.50| -0.27%   
  |   0.62%   |   1.12%| 1   | 5 |
| TPCH(60) | TPCH-Q13 | parquet / none / none | 22.11  | 22.18   | -0.31%   
  |   0.18%   |   0.55%| 1   | 5 |
| TPCH(60) | TPCH-Q15 | parquet / none / none | 8.45   | 8.48| -0.32%   
  |   0.40%   |   0.47%| 1   | 5 |
| TPCH(60) | TPCH-Q9  | parquet / none / none | 33.39  | 33.66   | -0.81%   
  |   0.75%   |   0.59%| 1   | 5 |
| TPCH(60) | TPCH-Q21 | parquet / none / none | 71.34  | 72.07   | -1.01%   
  |   1.84%   |   1.79%| 1   | 5 |
| TPCH(60) | TPCH-Q14 | parquet / none / none | 5.93   | 6.00| -1.07%   
  |   0.15%   |   0.69%| 1   | 5 |
| TPCH(60) | TPCH-Q20 | parquet / none / none | 5.72   | 5.79| -1.09%   
  |   0.59%   |   0.51%| 1   | 5 |
| TPCH(60) | TPCH-Q18 | parquet / none / none | 45.42  | 45.93   | -1.10%   
  |   1.42%   |   0.50%| 1   | 5 |
| TPCH(60) | TPCH-Q2  | parquet / none / none | 4.81   | 4.89| -1.52%   
  |   1.68%   |

[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu

2017-08-03 Thread Jim Apple (Code Review)

Jim Apple has posted comments on this change.

Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
..


Patch Set 1:

(1 comment)

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

PS1, Line 16: checking for both
Could we change the table property printing to be deterministic, rather than 
changing the test?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5572: Timestamp codegen for text scanner

2017-08-03 Thread Tianyi Wang (Code Review)

Tianyi Wang has uploaded a new patch set (#2).

Change subject: IMPALA-5572: Timestamp codegen for text scanner
..

IMPALA-5572: Timestamp codegen for text scanner

Currently codegen is disabled when scanning text tables with timestamp
columns. The message is "Timestamp not yet supported for codegen."
This patch adds support for timestamp codegen.
A simple query in the comment section of this issue performs a little
better (4%) than interpreted version.

Testing: The patch passed test with exhaustive workload exploration
strategy.

Change-Id: I00cbf8ec7784ca9594e14e952f46dc54a5ede44b
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/text-converter.cc
M be/src/util/string-parser.h
5 files changed, 33 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I00cbf8ec7784ca9594e14e952f46dc54a5ede44b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Lars Volker has posted comments on this change.

Change subject: IMPALA-2869: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 2:

(11 comments)

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

PS2, Line 7: why codegen is disabled in
How about "Log errors in"? "Disabled" reads as if it was done by a user on 
purpose.


PS2, Line 7: IMPALA-2869
Can you re-open the JIRA and assign it to yourself?


PS2, Line 10: propogate
typo


http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/text-converter.cc
File be/src/exec/text-converter.cc:

Line 76: //   %1 = bitcast <{ i32, i8 }>* %tuple_arg to i8*
Why did the IR change?


Line 111:   if (slot_desc->type().type == TYPE_CHAR) {
You could add a DCHECK(fn != nullptr)


PS2, Line 126: TextConverter::CodegenWriteSlot
Do you need this part? If the error gets logged, won't it include where it came 
from?


PS2, Line 127: a null string
>From the code and this error message it's not clear to me what went wrong. Can 
>you improve the error message?


Line 232: return Status("TextConverter::CodegenWriteSlot: Codegen'd 
parser NYI for the"
What does NYI mean? Maybe make this "Unsupported type". It should never be 
printed anyways and the Status will have a stack trace to find it.


Line 280: Status("TextConverter::CodegenWriteSlot:codegen'd "
This doesn't have any effect, did you forget a "return"?


Line 281:"WriteSlot function failed verification");
Maybe call it "FinalizeFunction() failed"? The error message seems different 
from what's actually happening (finalize vs verify).


http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/text-converter.h
File be/src/exec/text-converter.h:

Line 83:   /// errors.
Can you add a comment for the new output parameter?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5756: start memory maintenance thread after metric creation

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5756: start memory maintenance thread after metric 
creation
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb7337a68547b4d660f3100b04df2c20befb044c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-03 Thread anujphadke (Code Review)

anujphadke has uploaded a new patch set (#2).

Change subject: IMPALA-2869: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..

IMPALA-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Plumbing statuses through TextConverter::CodegenWriteSlot(), which
eventually propogate to the runtime profile.

Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
---
M be/src/exec/hdfs-scanner.cc
M be/src/exec/text-converter.cc
M be/src/exec/text-converter.h
3 files changed, 53 insertions(+), 41 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

Michael Ho has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5717/12/CMakeLists.txt
File CMakeLists.txt:

PS12, Line 153: 1.0.1
> To clarify, I meant to ask "does it actually require version 1.0.1 ? Will i
Actually, kudu itself seems to require 1.0.0 only: 
https://github.com/apache/kudu/blob/master/CMakeLists.txt#L881


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Impala-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-03 Thread anujphadke (Code Review)

anujphadke has uploaded a new change for review.

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

Change subject: Impala-2869: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..

Impala-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Plumbing statuses through TextConverter::CodegenWriteSlot(), which
eventually propogate to the runtime profile.

Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
---
M be/src/exec/hdfs-scanner.cc
M be/src/exec/text-converter.cc
M be/src/exec/text-converter.h
3 files changed, 52 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

Michael Ho has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5717/12/CMakeLists.txt
File CMakeLists.txt:

PS12, Line 153: 1.0.1
> Does it actually work with 1.0.0 ? libkudu_client.so seems to be able to fu
To clarify, I meant to ask "does it actually require version 1.0.1 ? Will it 
work with OpenSSL 1.0.0 ?"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

Michael Ho has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5717/12/CMakeLists.txt
File CMakeLists.txt:

PS12, Line 153: 1.0.1
Does it actually work with 1.0.0 ? libkudu_client.so seems to be able to 
function with OpenSSL version 1.0.0.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift

2017-08-03 Thread Sailesh Mukil (Code Review)

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
..


Patch Set 1:

(12 comments)

A lot of the comments are just about refactoring. Feel free to disagree with 
them if you think it's better the way it is.

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

PS1, Line 11: allows
nit: allow


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

Line 276: 
I would add one more test case where the server is started with a list of 
ciphers and the client is started with a different list of ciphers such that 
there is an overlap of a few ciphers; and test that things work as expected.

Eg:
Server has 3DES and RSA
Client has AES256 and RSA

Things should work because RSA is common between them?
(If the above is a valid example)


http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

PS1, Line 166:   friend class ThriftServerBuilder;
nit: Move just below private: as it's easier to find, plus that's the protocol 
usually followed to declare a friend.


PS1, Line 247: // Returns true if, per the process configuration flags, 
server<->server communications
 : // should use SSL.
 : bool EnableInternalSslConnections();
nit: Move below ThriftServerBuilder class or above ThriftServer class.

Good to have the builder and target class next to each other for readability.


PS1, Line 254: ThriftServerBuilder(const std::string& name,
 :   const boost::shared_ptr& 
processor, int port)
 : : name_(name), processor_(processor), port_(port) {}
Builder constructors usually just take the name from what I've seen in the past.
Everything else would ideally be set as an option.

Plus, that looks easier to read by knowing what options are being set at the 
caller side vs. passing members into a constructor.

The Build() function can return an error if the required parameters are not set.


PS1, Line 259: auth_provider
Since this is a builder class, I'd prefer if all these functions were prepended 
with "set_"

Eg:
set_auth_provider(), etc.


Line 311:   Status Build(ThriftServer** server) {
Thorough error checking needs to be done on the builder side before creating 
the target class object.

Eg: if (port_ == 0)

This is if you decide to change the constructor as I suggested above.


http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1947: be_builder.metrics(exec_env->metrics())
Set metrics just after constructor initialization in L1940 and just call 
be_builder.Build() here.


PS1, Line 1974: 
builder.auth_provider(AuthManager::GetInstance()->GetExternalAuthProvider())
  : .metrics(exec_env->metrics())
  : .thread_pool(FLAGS_fe_service_threads)
Same here, set all options just after constructor init and just call 
builder.Build() here.


PS1, Line 2000: 
builder.auth_provider(AuthManager::GetInstance()->GetExternalAuthProvider())
  : .metrics(exec_env->metrics())
  : .thread_pool(FLAGS_fe_service_threads)
  : .Build(hs2_server));
Same as above


http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

PS1, Line 203: ThriftServer* server;
 : RETURN_IF_ERROR(builder.Build());
 : heartbeat_server_.reset(server);
Surround with curly braces so that 'server' goes out of scope when it's not 
needed?


http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestored-main.cc
File be/src/statestore/statestored-main.cc:

PS1, Line 89: metrics(metrics.get())
move to L83.

Also follow this pattern everywhere else if you agree.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Revert "IMPALA-5658: addtl. process/system-wide memory metrics"

Tim Armstrong has abandoned this change.

Change subject: Revert "IMPALA-5658: addtl. process/system-wide memory metrics"
..


Abandoned

See https://gerrit.cloudera.org/#/c/7573/

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Iaa9317492c77279d79af770ef9a77df0e0914826
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5756: start memory maintenance thread after metric creation

2017-08-03 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5756: start memory maintenance thread after metric 
creation
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb7337a68547b4d660f3100b04df2c20befb044c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5756: start memory maintenance thread after metric creation

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5756: start memory maintenance thread after metric 
creation
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb7337a68547b4d660f3100b04df2c20befb044c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5756: start memory maintenance thread after metric creation