Dan Burkert has posted comments on this change. Change subject: KUDU-1812. Redact pretty-printed sensitive user data ......................................................................
Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/5555/2/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: Line 524: VLOG(3) << "Looking up tablet for " << KUDU_REDACT(op->write_op->ToString()); > isn't this ToString() already safe? This is not safe, because the KuduWriteOperation::ToString is not safe. I'm going to switch everything to use the safe InFlightOp::ToString Line 574: << "Could not remove op " << KUDU_REDACT(op->ToString()) << " from in-flight list"; > same (and several below) This one is safe because it's calling the InFlightOp::ToString, which is redacted. http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 1284: } else if (data_->last_response_.has_more_results()) { > warning: don't use else after return [readability-else-after-return] Ignoring. http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/client/client.h File src/kudu/client/client.h: PS1, Line 1926: /// : /// @internal This method must not be used in log messages because it contains : /// sensi > OK, I see now. That's perfect, I'll use that instead. http://gerrit.cloudera.org:8080/#/c/5555/2/src/kudu/client/client.h File src/kudu/client/client.h: Line 2115 > hrm, was this ToString never implemented? correct http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/client/write_op.h File src/kudu/client/write_op.h: PS1, Line 78: /// : /// @internal this method does *NOT* redact row values. The > nit: since it's 'doxygenized' comment, please make the new note to conform Done http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: Line 36: using std::get; > warning: using decl 'all_of' is unused [misc-unused-using-decls] Done Line 468: } > warning: std::move of the const expression has no effect; remove std::move( Done http://gerrit.cloudera.org:8080/#/c/5555/2/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: Line 466: VLOG(1) << "Pushing down predicate " << KUDU_REDACT(pred.ToString()); > would be nice to keep the column name, operator, etc here Done http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: PS1, Line 461: /// @internal this method does *NOT* redact row values. The : /// caller must check 'log_row_contents' themselves, if applicab > nit: since it's 'doxygenized' comment, please make the new note to conform Done http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/common/schema.h File src/kudu/common/schema.h: PS1, Line 28: <glog/logging.h> > I'm not sure this is needed in this file. If yes, then as a nit, please co Done http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: Line 119: virtual Status DebugDump(vector<string> *lines = NULL) = 0; > warning: default arguments on virtual or override methods are prohibited [g Ignoring. http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/tablet/transactions/alter_schema_transaction.cc File src/kudu/tablet/transactions/alter_schema_transaction.cc: Line 43: > warning: using decl 'AlterSchemaResponsePB' is unused [misc-unused-using-de Done http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/tablet/transactions/transaction.h File src/kudu/tablet/transactions/transaction.h: Line 122: virtual void Finish(TransactionResult result) {} > warning: parameter 'result' is unused [misc-unused-parameters] ignoring. http://gerrit.cloudera.org:8080/#/c/5555/1/src/kudu/tools/tool_main.cc File src/kudu/tools/tool_main.cc: Line 46: using std::deque; > warning: using decl 'cout' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/5555/2/src/kudu/util/logging.h File src/kudu/util/logging.h: Line 180: // as a macro so that the message can be lazily evaluated. > I think lazy evaluation is good in the context of a release build, but I'm You have kept this as a macro in your version, so I'm assuming the default log reaction beign off is good enough. -- 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: 2 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
