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 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337 PS6, Line 337: if (GetLocalNetworks(&local_networks).ok()) { > I think, effectively, this is a hard stop to the current test case. If GetL That's not a hard stop though; by skipping the remainder of the test, it'll be marked as PASSED. A hard stop would be an ASSERT failure that'd mark the test as FAILED. Or a CHECK failure that crashes the test. Put another way: does !GetLocalNetworks.ok() signify an unexpected error in the platform environment? Or an error that might be expected and fully deterministic on some platforms? Based on a reading of the GetLocalNetworks code, it looks like the former to me, in which case we should treat it the same way as e.g. a failure to read a test file: fail the test. http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342 PS6, Line 342: char s[INET_ADDRSTRLEN]; : inet_ntop(AF_INET, &addr, s, INET_ADDRSTRLEN); > I wasn't sure if creating new members inside Network is warranted until the Not really. My view is that I want as much platform-specific code (i.e. all those headers you had to add) out of random tests, and squirreled away behind util classes. The Network class purports to provide an abstraction for interacting with an IPv4 network; adding more methods to it rounds out the abstraction and makes it more useful for future use cases. It's especially non-concerning given that the two methods I suggested are read-only; they just provide slightly different views/transforms of the abstraction. -- 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: 6 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: Fri, 15 Feb 2019 00:51:02 +0000 Gerrit-HasComments: Yes
