impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] PREVIEW: IMPALA-2615: support [[nodiscard]] on Status
Date Fri, 11 Aug 2017 01:41:02 GMT
Tim Armstrong has posted comments on this change.

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

Patch Set 5:

File be/CMakeLists.txt:

> 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.
File be/src/exec/

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:

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.
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.
File be/src/rpc/auth-provider.h:

Line 51:   /// that
> fix wrap
File be/src/rpc/

Line 40:   if (!init_status_.ok()) return init_status_;
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.
File be/src/runtime/

Line 346:   Status status = mgr_->DeregisterRecvr(fragment_instance_id(), dest_node_id());
> const
File be/src/runtime/

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
File be/src/runtime/

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.
File be/src/scheduling/

Line 510:   Status status = scheduler_->Init();
> const
File be/src/service/

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.
File be/src/service/

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

To view, visit
To unsubscribe, visit

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

View raw message