Alexey Serbin has posted comments on this change. Change subject: KUDU-1812. Redact pretty-printed sensitive user data ......................................................................
Patch Set 6: (11 comments) Woops, I accidentally pressed wrong button :( My bad -- I'm sorry. I was about to press the 'Post' button instead. My comments are mostly nits, though, but I remember Todd about to look at the patch as well. http://gerrit.cloudera.org:8080/#/c/5555/4//COMMIT_MSG Commit Message: PS4, Line 7: . nit: strayed dot? consider removing PS4, Line 10: nit: it would be nice to note that it's about status output and logging. May be, something like: ... to be redacted when output into logs or in status messages. That's done in the Kudu server side and, ... PS4, Line 14: e nit: here and elsewhere in the commit message -- consider wrapping up at 72 chars, if possible PS4, Line 42: doxygen has nit: 'doxygen entries have been updated' or 'doxygen docs have been updated' ? http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/client/client.h File src/kudu/client/client.h: PS4, Line 1927: @internal nit: I would expect that to be under '@attention' or '@note' flag. Also, consider moving that before the @return: usually, @return is the last entry in those blocks. I.e., something like that: @internal @attention This method ... @endinternal or @internal @note This method ... @endinternal http://gerrit.cloudera.org:8080/#/c/5555/5/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: Line 23: nit: consider adding <gflags/gflags_declare.h> and re-ordering the includes. http://gerrit.cloudera.org:8080/#/c/5555/5/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: PS5, Line 462: NOTE: nit: replace with @note for the doxygen-like style http://gerrit.cloudera.org:8080/#/c/5555/5/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: PS5, Line 26: gflags.h nit: gflags_declare.h would suffice PS5, Line 457: // Explicitly enable redaction. It should have no effect on the subsequent : // partition pretty printing tests, as partitions are metadata and thus not : // redacted. I'm not sure the order of tests for this class is always the same as in the source file -- I know there might be cases when it's not so: there is explicit --gtest_shuffle flag which might be used. Consider setting the flag explicitly in every test. http://gerrit.cloudera.org:8080/#/c/5555/5/src/kudu/common/types-test.cc File src/kudu/common/types-test.cc: PS5, Line 37: TestTypes nit: it seems the modified AppendDebugStringForValue() method has no appropriate coverage in this test for the case when reduction is enabled. Is it worth adding a small test here -- just to verify how the method works in redaction-enabled case. http://gerrit.cloudera.org:8080/#/c/5555/5/src/kudu/util/logging.h File src/kudu/util/logging.h: PS5, Line 63: "redacted" nit: it seems to be "<redacted>" -- To view, visit http://gerrit.cloudera.org:8080/5555 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b87a3065280116bb8af6f26f072dafdfd1ee077 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
