impala-reviews mailing list archives

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

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

Patch Set 6:

File be/CMakeLists.txt:

> My thought was that it's only setting GCC flags, so it shouldn't matter.
I find it is a confusing pattern.

Line 74:   #  -Wno-placement-new: avoid spurious warnings from boost::function.
> Added a link to the Boost fix for it.
Are those warnings really spurious? I can't tell immediately from the link.

That's not to say they need to be fixed in this commit, but only that if these warnings are
serious, we should file a ticket.

Line 75:   #   TODO: remove when we upgrade Boost:
> Filed KUDU-2094 against Kudu. It looks like Counter contains class Cell, wh
Thanks for looking into that.

I'm torn on whether this patch should hold on getting these two issues (boost and kudu) fixed.
On the one hand, waiting would allow you to forgo adding the -Wno- flags, and would ensure
we don't accidentally leave them in forever, obscuring legit warnings.

OTOH, a patch this large can become outdated and difficult to commit cleanly if it has to
wait. What do you think?
File be/src/statestore/statestore.h:

Line 443:   /// the
fix wrap
File be/src/testutil/

Line 71:       Status status = impala->SetCatalogInitialized();
File be/src/util/

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) or abort when errors
are encountered?
File be/src/util/

Line 748:       MakeScopeExitTrigger([&compressor]() { compressor->Close(); });
Should Close() be added to the destructor (after making it unipotent)?
File be/src/util/runtime-profile.h:

Line 268:   Status SerializeToArchiveString(std::string* out) const WARN_UNUSED_RESULT;
Makes the case for a Maybe<T> template class that can hold a T or a Status. Shall we
file a ticket for this, or do you feel it is too niche?

To view, visit
To unsubscribe, visit

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

View raw message