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 <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
