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/in-process-servers.cc:

Line 71:       Status status = impala->SetCatalogInitialized();

File be/src/util/parquet-reader.cc:

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/runtime-profile.cc:

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 
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 http://gerrit.cloudera.org:8080/7253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 6
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