[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


IMPALA-2615: support [[nodiscard]] on Status

This is the set of changes required to get Impala to compile on
GCC 7 using the [[nodiscard]] attribute, which generates a warning
whenever a status is dropped. It is not enabled on the current
default compiler GCC 4.9.2 or Clang 3.8 so I added WARN_UNUSED_RESULT
in various classes so that we can catch the dropped statuses with
our current toolchain.

The changes are:
* Use the new [[nodiscard]] attribute and fix all the dropped
  statuses. Many were innocuous or very improbably but some appear to
  be actual bugs. Adds a discard_result() function that explicitly
  ignores the result of a function.
* Removes the bad JNI pattern of checking for exceptions after
  DeleteGlobalRef(), which doesn't throw.
* Fix miscellaneous compile errors and warnings.
* Remove use of ptr_vector, which pulls in headers with deprecated
  things.
* Fix a memory lifetime bug with default_fs_ (it was masked by
  the old refcounted std::string implementation).

Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Reviewed-on: http://gerrit.cloudera.org:8080/7253
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalogd-main.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/common/compiler-util.h
M be/src/common/init.cc
M be/src/common/status.h
M be/src/exec/external-data-source-executor.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hbase-table-scanner.cc
M be/src/exec/hbase-table-scanner.h
M be/src/exec/hbase-table-writer.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/kudu-util.h
M be/src/experiments/compression-test.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/collection-value-builder.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/hbase-table-factory.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/parallel-executor.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test.cc
M be/src/service/client-request-state.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/service/query-options-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/statestore/statestored-main.cc
M be/src/testutil/death-test-util.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/impalad-query-executor.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/codec.h
M be/src/util/filesystem-util.h
M be/src/util/hdfs-util-test.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/network-util.h
M be/src/util/parquet-reader.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/thread.cc
M be/src/util/thread.h
82 files changed, 403 insertions(+), 309 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 15: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 15:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 15: Code-Review+2

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 14:

(1 comment)

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

PS14, Line 211: StartServices
> fwiw, I have a patch that breaks this out into Init() and StartServices() (
Sounds good.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 14:

(1 comment)

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

PS14, Line 211: StartServices
> this name seems kinda weird for what it does. Should this just be called In
fwiw, I have a patch that breaks this out into Init() and StartServices() (I 
think the latter is useful for starting webservers, statestore subscribers and 
the like). Having the separation was useful for the KRPC initialization logic. 
https://gerrit.cloudera.org/#/c/7673/


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 14:

(1 comment)

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

PS14, Line 211: StartServices
> this name seems kinda weird for what it does. Should this just be called In
Henry has a patch to do exactly this. https://gerrit.cloudera.org/#/c/7673/


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 14: Code-Review+2

(1 comment)

Seems okay to me, though maybe we could improve the name of StartServices(). It 
doesn't make a lot of sense to me, what do you think?

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

PS14, Line 211: StartServices
this name seems kinda weird for what it does. Should this just be called Init()?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 14:

It would be good if someone could take a quick look at the PS13->14 delta to 
make sure that the fix makes sense.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 13:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 14:

There were some side-effects of changing the startup order. Updated with a less 
invasive solution - just don't try to register with the statestore if it's a 
test.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Hello Impala Public Jenkins, Jim Apple, Dan Hecht,

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

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

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

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..

IMPALA-2615: support [[nodiscard]] on Status

This is the set of changes required to get Impala to compile on
GCC 7 using the [[nodiscard]] attribute, which generates a warning
whenever a status is dropped. It is not enabled on the current
default compiler GCC 4.9.2 or Clang 3.8 so I added WARN_UNUSED_RESULT
in various classes so that we can catch the dropped statuses with
our current toolchain.

The changes are:
* Use the new [[nodiscard]] attribute and fix all the dropped
  statuses. Many were innocuous or very improbably but some appear to
  be actual bugs. Adds a discard_result() function that explicitly
  ignores the result of a function.
* Removes the bad JNI pattern of checking for exceptions after
  DeleteGlobalRef(), which doesn't throw.
* Fix miscellaneous compile errors and warnings.
* Remove use of ptr_vector, which pulls in headers with deprecated
  things.
* Fix a memory lifetime bug with default_fs_ (it was masked by
  the old refcounted std::string implementation).

Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalogd-main.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/common/compiler-util.h
M be/src/common/init.cc
M be/src/common/status.h
M be/src/exec/external-data-source-executor.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hbase-table-scanner.cc
M be/src/exec/hbase-table-scanner.h
M be/src/exec/hbase-table-writer.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/kudu-util.h
M be/src/experiments/compression-test.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/collection-value-builder.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/hbase-table-factory.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/parallel-executor.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test.cc
M be/src/service/client-request-state.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/service/query-options-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/statestore/statestored-main.cc
M be/src/testutil/death-test-util.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/impalad-query-executor.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/codec.h
M be/src/util/filesystem-util.h
M be/src/util/hdfs-util-test.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/network-util.h
M be/src/util/parquet-reader.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/thread.cc
M be/src/util/thread.h
82 files changed, 403 insertions(+), 309 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 13:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 13:

expr-test failed for a different reason this time that needs investigation.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 13: Verified-1

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 13:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 13: Code-Review+2

Rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 12: Code-Review+2

The bug is that the in-process servers start the statestore subscriber before 
registering topics, which is invalid. The real servers do it in the right order.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Hello Impala Public Jenkins, Jim Apple, Dan Hecht,

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

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

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

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..

IMPALA-2615: support [[nodiscard]] on Status

This is the set of changes required to get Impala to compile on
GCC 7 using the [[nodiscard]] attribute, which generates a warning
whenever a status is dropped. It is not enabled on the current
default compiler GCC 4.9.2 or Clang 3.8 so I added WARN_UNUSED_RESULT
in various classes so that we can catch the dropped statuses with
our current toolchain.

The changes are:
* Use the new [[nodiscard]] attribute and fix all the dropped
  statuses. Many were innocuous or very improbably but some appear to
  be actual bugs. Adds a discard_result() function that explicitly
  ignores the result of a function.
* Removes the bad JNI pattern of checking for exceptions after
  DeleteGlobalRef(), which doesn't throw.
* Fix miscellaneous compile errors and warnings.
* Remove use of ptr_vector, which pulls in headers with deprecated
  things.
* Fix a memory lifetime bug with default_fs_ (it was masked by
  the old refcounted std::string implementation).

Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalogd-main.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/common/compiler-util.h
M be/src/common/init.cc
M be/src/common/status.h
M be/src/exec/external-data-source-executor.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hbase-table-scanner.cc
M be/src/exec/hbase-table-scanner.h
M be/src/exec/hbase-table-writer.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/kudu-util.h
M be/src/experiments/compression-test.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/collection-value-builder.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/hbase-table-factory.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/parallel-executor.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test.cc
M be/src/service/client-request-state.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/service/query-options-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/statestore/statestored-main.cc
M be/src/testutil/death-test-util.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/impalad-query-executor.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/codec.h
M be/src/util/filesystem-util.h
M be/src/util/hdfs-util-test.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/network-util.h
M be/src/util/parquet-reader.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/thread.cc
M be/src/util/thread.h
82 files changed, 401 insertions(+), 308 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 11: Verified-1

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 11: Code-Review+2

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 11:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Hello Jim Apple, Dan Hecht,

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

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

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

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..

IMPALA-2615: support [[nodiscard]] on Status

This is the set of changes required to get Impala to compile on
GCC 7 using the [[nodiscard]] attribute, which generates a warning
whenever a status is dropped. It is not enabled on the current
default compiler GCC 4.9.2 or Clang 3.8 so I added WARN_UNUSED_RESULT
in various classes so that we can catch the dropped statuses with
our current toolchain.

The changes are:
* Use the new [[nodiscard]] attribute and fix all the dropped
  statuses. Many were innocuous or very improbably but some appear to
  be actual bugs. Adds a discard_result() function that explicitly
  ignores the result of a function.
* Removes the bad JNI pattern of checking for exceptions after
  DeleteGlobalRef(), which doesn't throw.
* Fix miscellaneous compile errors and warnings.
* Remove use of ptr_vector, which pulls in headers with deprecated
  things.
* Fix a memory lifetime bug with default_fs_ (it was masked by
  the old refcounted std::string implementation).

Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalogd-main.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/common/compiler-util.h
M be/src/common/init.cc
M be/src/common/status.h
M be/src/exec/external-data-source-executor.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hbase-table-scanner.cc
M be/src/exec/hbase-table-scanner.h
M be/src/exec/hbase-table-writer.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/kudu-util.h
M be/src/experiments/compression-test.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/collection-value-builder.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/hbase-table-factory.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/parallel-executor.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test.cc
M be/src/service/client-request-state.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/service/query-options-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/statestore/statestored-main.cc
M be/src/testutil/death-test-util.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/impalad-query-executor.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/codec.h
M be/src/util/filesystem-util.h
M be/src/util/hdfs-util-test.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/network-util.h
M be/src/util/parquet-reader.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/thread.cc
M be/src/util/thread.h
82 files changed, 399 insertions(+), 307 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Hello Jim Apple, Dan Hecht,

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

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

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

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..

IMPALA-2615: support [[nodiscard]] on Status

This is the set of changes required to get Impala to compile on
GCC 7 using the [[nodiscard]] attribute, which generates a warning
whenever a status is dropped. It is not enabled on the current
default compiler GCC 4.9.2 or Clang 3.8 so I added WARN_UNUSED_RESULT
in various classes so that we can catch the dropped statuses with
our current toolchain.

The changes are:
* Use the new [[nodiscard]] attribute and fix all the dropped
  statuses. Many were innocuous or very improbably but some appear to
  be actual bugs. Adds a discard_result() function that explicitly
  ignores the result of a function.
* Removes the bad JNI pattern of checking for exceptions after
  DeleteGlobalRef(), which doesn't throw.
* Fix miscellaneous compile errors and warnings.
* Remove use of ptr_vector, which pulls in headers with deprecated
  things.
* Fix a memory lifetime bug with default_fs_ (it was masked by
  the old refcounted std::string implementation).

Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalogd-main.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/common/compiler-util.h
M be/src/common/init.cc
M be/src/common/status.h
M be/src/exec/external-data-source-executor.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hbase-table-scanner.cc
M be/src/exec/hbase-table-scanner.h
M be/src/exec/hbase-table-writer.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/kudu-util.h
M be/src/experiments/compression-test.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/collection-value-builder.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/hbase-table-factory.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/parallel-executor.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test.cc
M be/src/service/client-request-state.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/service/query-options-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/statestore/statestored-main.cc
M be/src/testutil/death-test-util.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/impalad-query-executor.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/codec.h
M be/src/util/filesystem-util.h
M be/src/util/hdfs-util-test.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/network-util.h
M be/src/util/parquet-reader.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/thread.cc
M be/src/util/thread.h
83 files changed, 401 insertions(+), 309 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 10: Code-Review+2

(1 comment)

Rebase

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

Line 748:   MakeScopeExitTrigger([]() { compressor->Close(); });
> i prefer goto, but up to you.
Ok, I'll leave as-is, seems slightly less error-prone in this context.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 9:

(1 comment)

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

Line 748:   MakeScopeExitTrigger([]() { compressor->Close(); });
> I couldn't think of a great alternative. Putting Close() before every retur
i prefer goto, but up to you.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 9:

(1 comment)

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

Line 748:   MakeScopeExitTrigger([]() { compressor->Close(); });
> i'm not a fan of this pattern, but ok.
I couldn't think of a great alternative. Putting Close() before every return is 
error-prone. I could switch to goto plus a "cleanup:" block if you prefer.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 9:

Yeah we could drop it for Status return values. We would keep it for bool or 
other return values (there are a few cases)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 9: Code-Review+2

(1 comment)

So once nodiscard is supported everywhere, would we drop the 
WARN_UNUSED_RESULTs?

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

Line 748:   MakeScopeExitTrigger([]() { compressor->Close(); });
i'm not a fan of this pattern, but ok.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 9: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7253/5/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 74:   #  -Wno-placement-new: avoid spurious warnings from boost::function.
> I'm not thrilled to be turning off this warning. I expect it will be hard t
I was able to suppress it in a more standard way by marking boost as a system 
header. I tried to do this before but didn't find both places it needed to 
change.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..

IMPALA-2615: support [[nodiscard]] on Status

This is the set of changes required to get Impala to compile on
GCC 7 using the [[nodiscard]] attribute, which generates a warning
whenever a status is dropped. It is not enabled on the current
default compiler GCC 4.9.2 or Clang 3.8 so I added WARN_UNUSED_RESULT
in various classes so that we can catch the dropped statuses with
our current toolchain.

The changes are:
* Use the new [[nodiscard]] attribute and fix all the dropped
  statuses. Many were innocuous or very improbably but some appear to
  be actual bugs. Adds a discard_result() function that explicitly
  ignores the result of a function.
* Removes the bad JNI pattern of checking for exceptions after
  DeleteGlobalRef(), which doesn't throw.
* Fix miscellaneous compile errors and warnings.
* Remove use of ptr_vector, which pulls in headers with deprecated
  things.
* Fix a memory lifetime bug with default_fs_ (it was masked by
  the old refcounted std::string implementation).

Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalogd-main.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/common/compiler-util.h
M be/src/common/init.cc
M be/src/common/status.h
M be/src/exec/external-data-source-executor.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hbase-table-scanner.cc
M be/src/exec/hbase-table-scanner.h
M be/src/exec/hbase-table-writer.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/kudu-util.h
M be/src/experiments/compression-test.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/collection-value-builder.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/hbase-table-factory.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/parallel-executor.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test.cc
M be/src/service/client-request-state.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/service/query-options-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/statestore/statestored-main.cc
M be/src/testutil/death-test-util.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/impalad-query-executor.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/codec.h
M be/src/util/filesystem-util.h
M be/src/util/hdfs-util-test.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/network-util.h
M be/src/util/parquet-reader.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/thread.cc
M be/src/util/thread.h
83 files changed, 399 insertions(+), 307 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7253/5/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 74:   # We need to add additional arguments for GCC 7+. We go down this 
branch if building
> It seems ok - the memory region written is large enough. The function_buffe
I'm not thrilled to be turning off this warning. I expect it will be hard to 
turn it back on. What if we switched boost::function to std::function before 
this patch?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 6:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/7253/5/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 73: if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 7.0)
> I find it is a confusing pattern.
I don't actually see a reasonable alternative that doesn't involve extra 
redundant code or a lot of refactoring. The current approach is to build up the 
argument strings then switch on ${CMAKE_BUILD_TYPE} below to determine whether 
it's a GCC or Clang build.

I added a comment to explain why it works. We can remove the branch and clean 
it up when we upgrade GCC.


Line 74:   #  -Wno-placement-new: avoid spurious warnings from boost::function.
> Are those warnings really spurious? I can't tell immediately from the link.
It seems ok - the memory region written is large enough. The function_buffer 
union is large enough at 8 bytes, it's just the data member that is too small 
at 1 byte.


Line 75:   #   TODO: remove when we upgrade Boost: 
https://github.com/boostorg/function/pull/9
> Thanks for looking into that.
-faligned-new I think is fine, since it's enabling improved behaviour that is 
part of the C++17 standard instead of masking an issue.

I don't think we should let upgrading Boost block upgrading GCC - I have a 
feeling that the Boost upgrade could be a lot of work.


http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

Line 443:   /// the
> fix wrap
Done


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

Line 71:   Status status = impala->SetCatalogInitialized();
> const
Done


http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/util/parquet-reader.cc
File be/src/util/parquet-reader.cc:

Line 138: // Returns the number of rows specified by the header.
> pre-existing issue, but: can you add a note that this function can exit(1) 
Done


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

Line 748:   MakeScopeExitTrigger([]() { compressor->Close(); });
> Should Close() be added to the destructor (after making it unipotent)?
We've been trying to standardise elsewhere on explicit resource release with 
trivial destructors. The argument is that implicit sequencing of non-trivial 
teardown steps is very hard to understand.

I put up a page in the wiki that documents some of the patterns in the code: 
https://cwiki.apache.org/confluence/display/IMPALA/Resource+Management+Best+Practices+in+Impala


http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

Line 268:   Status SerializeToArchiveString(std::string* out) const 
WARN_UNUSED_RESULT;
> Makes the case for a Maybe template class that can hold a T or a Status.
I don't feel strongly. I like those types in other languages but they don't 
seem to work as well in C++ where there isn't a convenient language construct 
to branch on the result and unpack the value.

We have to use LLVM's ErrorOr in a couple of places and IMO the code wasn't 
simpler than using an output argument.

It looks like C++17 has std::optional, which is available earlier as 
std::experimental::optional 
http://en.cppreference.com/w/cpp/experimental/optional


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..

IMPALA-2615: support [[nodiscard]] on Status

This is the set of changes required to get Impala to compile on
GCC 7 using the [[nodiscard]] attribute, which generates a warning
whenever a status is dropped. It is not enabled on the current
default compiler GCC 4.9.2 or Clang 3.8 so I added WARN_UNUSED_RESULT
in various classes so that we can catch the dropped statuses with
our current toolchain.

The changes are:
* Use the new [[nodiscard]] attribute and fix all the dropped
  statuses. Many were innocuous or very improbably but some appear to
  be actual bugs. Adds a discard_result() function that explicitly
  ignores the result of a function.
* Removes the bad JNI pattern of checking for exceptions after
  DeleteGlobalRef(), which doesn't throw.
* Fix miscellaneous compile errors and warnings.
* Remove use of ptr_vector, which pulls in headers with deprecated
  things.
* Fix a memory lifetime bug with default_fs_ (it was masked by
  the old refcounted std::string implementation).

Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
---
M be/CMakeLists.txt
M be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalogd-main.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/common/compiler-util.h
M be/src/common/init.cc
M be/src/common/status.h
M be/src/exec/external-data-source-executor.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hbase-table-scanner.cc
M be/src/exec/hbase-table-scanner.h
M be/src/exec/hbase-table-writer.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/kudu-util.h
M be/src/experiments/compression-test.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/collection-value-builder.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/hbase-table-factory.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/parallel-executor.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test.cc
M be/src/service/client-request-state.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/service/query-options-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/statestore/statestored-main.cc
M be/src/testutil/death-test-util.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/impalad-query-executor.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/codec.h
M be/src/util/filesystem-util.h
M be/src/util/hdfs-util-test.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/network-util.h
M be/src/util/parquet-reader.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/thread.cc
M be/src/util/thread.h
82 files changed, 397 insertions(+), 305 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong