Tim Armstrong has posted comments on this change.

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


Patch Set 5:

(14 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)
> Should this be limited to GCC, too?
My thought was that it's only setting GCC flags, so it shouldn't matter.


Line 74:   #  -Wno-placement-new: avoid spurious warnings from boost::function.
> Can you describe this error a bit more?
Added a link to the Boost fix for it.


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
Filed KUDU-2094 against Kudu. It looks like Counter contains class Cell, which 
is meant to be cacheline aligned. So this is a bug in the Kudu util code.


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 y
This triggered me to look more closely at why DeleteGlobalRef might throw an 
exception. It looks like it can't actually throw an exception (there is no 
THROWS section here: 
http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html)

The pattern of checking for exceptions seems to have started in this commit: 
85b1154ba1c2edcccd673eb92c4d30c9a635442e where the exception checked for was 
actually from the method call.

Then it got copy-pasted and was made into a utility function.


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?
For Status at least it will be automatic. We would just use it for other return 
types. E.g. bool.


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
Done


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?
Done


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?
I think that should be up to the caller.


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
Done


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?
It doesn't matter if prepare failed since we're cancelling it


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?
We can't propagate the status here.

We also shouldn't be running this function outside of backend tests, since the 
HBaseTableFactory is a singleton that's only set up once per daemon process.


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
Done


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.
This propagates a Java exception to the Java code calling into this function 
via JNI. It's done elsewhere in this file.


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
Done


-- 
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 <tarmstr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to