Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test ...................................................................... Patch Set 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@330 PS10, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(&local_networks), ERROR, > My reason for using KUDU_RETURN_NOT_OK_LOG instead of just KUDU_RETURN_NOT_ Let's take an example: an operation whose execution makes a tree-like graph of function calls. Every function called returns a Status, and every call site is wrapped in RETURN_NOT_OK. The top-most node in the tree is the function that began the operation. It does not wrap the first call with RETURN_NOT_OK; instead, it stores the result in a local Status variable, and LOGs it, converts it into an RPC response, or does something else with it. Why this structure? The idea is for all the intermediate layers to act as conduits for a bad Status, leaving the logging up to the root. This makes sense; it is best positioned to know whether logging is appropriate or not. If every layer wrapped in _LOG, there'd be a lot of duplicate logging for this one operation. Moreover, trying to make good logging decisions in intermediate layers is hard because they're often reused for different purposes by different code paths, some of which may want logging and some of which don't. Pushing the bad Status up the call stack and delegating the logging decision to your caller is the best way to ensure that the right decision is always made. An analogy here is Java's exception handling: when you throw an exception, you only catch it when you want to handle it and/or log; you wouldn't typically catch it, log, then rethrow. In this particular case, _LOG will trigger duplicate logging: once here, and once on L461. As for RETURN_NOT_OK_PREPEND, we use that when we want an intermediate layer to add information to the bad Status. It won't affect whether a bad Status is logged or not; it'll just modify the message that gets logged. In this case, if L461 logs, it'll very obviously be either from L342 (if the message contains "Could not find an external IP") or L330 (some other message), so _PREPEND doesn't seem necessary inasfar as identifying the true error source. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 13 Gerrit-Owner: Greg Solovyev <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 16 Feb 2019 04:30:47 +0000 Gerrit-HasComments: Yes
