Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201708110002.v7B0249L025583@ip-10-146-233-104.ec2.internal>
Date: Fri, 11 Aug 2017 00:02:04 +0000
From: "Jim Apple (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Jim Apple <jbapple-impala@apache.org>
Reply-To: jbapple-impala@apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_PREVIEW=3A_IMPALA-2615=3A_support_=5B=5Bnodiscard=5D=5D_on_Status=0A?=
In-Reply-To: <gerrit.1498083447000.I972543af2e9f98b12dcbb5479b4c1a7d53952197@gerrit.cloudera.org>
References: <gerrit.1498083447000.I972543af2e9f98b12dcbb5479b4c1a7d53952197@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Fri, 11 Aug 2017 00:02:12 -0000

Jim Apple has posted comments on this change.

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


Patch Set 5:

(14 comments)

Halfway through reviewing; though we should hash some of this out before I do the second half.

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)
Should this be limited to GCC, too?


Line 74:   #  -Wno-placement-new: avoid spurious warnings from boost::function.
Can you describe this error a bit more?


Line 75:   #  -faligned-new: new will automatically align types. Otherwise "new Counter()" in the
It is my understanding that new produces things that are as aligned as void * (aka 8 bytes). Can you describe a bit more what's going on with this warning?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/exec/hbase-table-scanner.cc
File be/src/exec/hbase-table-scanner.cc:

Line 764:     discard_result(JniUtil::FreeGlobalRef(env, resultscanner_));
What is the rationale for the difference between this line and 761, where you LOG(WARNING)? See also below.


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/exec/hbase-table-scanner.h
File be/src/exec/hbase-table-scanner.h:

Line 88:   static Status Init() WARN_UNUSED_RESULT;
Once we move to GCC7, we no longer need WARN_UNUSED_RESULT?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/auth-provider.h
File be/src/rpc/auth-provider.h:

Line 51:   /// that
fix wrap


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/thrift-client.cc
File be/src/rpc/thrift-client.cc:

Line 40:   if (!init_status_.ok()) return init_status_;
RETURN_IF_ERROR?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/thrift-client.h
File be/src/rpc/thrift-client.h:

Line 161:   if (!init_status_.ok()) return;
and LOG?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/runtime/data-stream-recvr.cc
File be/src/runtime/data-stream-recvr.cc:

Line 346:   Status status = mgr_->DeregisterRecvr(fragment_instance_id(), dest_node_id());
const


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

Line 100:   discard_result(WaitForPrepare());  // make sure Prepare() finished
Why is discard OK here?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/runtime/hbase-table-factory.cc
File be/src/runtime/hbase-table-factory.cc:

Line 95:     discard_result(JniUtil::FreeGlobalRef(env, connection_));
Why is discard OK here?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/scheduling/scheduler-test-util.cc
File be/src/scheduling/scheduler-test-util.cc:

Line 510:   Status status = scheduler_->Init();
const


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

Line 72:   THROW_IF_ERROR(LlvmCodeGen::InitializeLlvm(true), env, JniUtil::internal_exc_class());
I'm surprised you want to throw in a function with extern "C" linkage.


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

Line 697:     Status status = profile_logger_->Flush();
const


-- 
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-HasComments: Yes