You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2017/02/22 20:19:19 UTC

[kudu-CR] Combine redaction flag for log and flags into one. 

Hao Hao has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6112

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with 'log'
and 'flag', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
14 files changed, 50 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6112/11/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

PS11, Line 145: enabledRedactTypes
Nit: this should be enabled_redact_types.


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 9:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:

Line 1087:   ASSERT_NE("", gflags::SetCommandLineOption("redact", "log"));
> So this reruns the validator? Is that how the log boolean gets reset?
Yeah, according to gflags doc, SetCommandLineOption would trigger validator.


http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 150:   vector<string>::const_iterator iter;
> You can use an enhanced for loop:
Done


Line 165:   if (!is_valid) {
> Why don't we store the result of FLAG redaction in a global variable too? T
Done


http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/util/flags.h
File src/kudu/util/flags.h:

Line 68: #endif /* KUDU_UTIL_FLAGS_H */
> Why do we still need this? Either logging.h should call this and should_red
Done


http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:

Line 80: bool g_should_redact_log;
> Does this actually need a default value, since it's always initialized via 
Done


http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/util/logging.h
File src/kudu/util/logging.h:

Line 86: extern bool g_should_redact_log;
> Nit: we generally prefix global variables with g_.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 11: Code-Review+2

Looks good to me, maybe see if Todd wants to rereview?

-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#3).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with 'log'
and 'flag', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 61 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#9).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with
'all', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 97 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/9
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#6).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with 'log'
and 'flag', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 113 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/6
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#11).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with
'all', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 98 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/11
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 15: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#10).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with
'all', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 98 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/10
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6112/10/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 327: bool g_should_redact_flag;
Let's also declare this one in the header, so it's even more consistent with g_should_redact_log. It'll make it easier for someone else to see what they need to do when adding a new redaction type in the future.


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 12: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6112/10/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 327: bool g_should_redact_flag;
> Let's also declare this one in the header, so it's even more consistent wit
You mean flags.h, right? I have already put it there. Or I missed something?


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6112/10/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 327: bool g_should_redact_flag;
> You mean flags.h, right? I have already put it there. Or I missed something
Sorry, I confused myself. What I meant was, can we declare g_should_redact_log and g_should_redact_flag in the exact same way? This would mean declaring them extern in the same header and defining them in the same .cc file.


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 3:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 112: DEFINE_string(redact, "log,flag",
> Please use a gflag validator (google::RegisterFlagValidator) to validate th
Done


Line 113:               "Sensitive information that needs to be redacted (comma-separated "
> How about: "Comma-separated list of redactions. Supported redactions are 'f
Done


PS3, Line 115: flags tagged as 'sensitive'will
> this is somewhat uninterpretable to users, since they don't really know abo
Done


Line 118:               "redacted.");
> can we also add an "all" or "*" mode?
Done


PS3, Line 275: redactLog
> style: REDACT_LOG or kRedactLog or something
Done


Line 488: bool ShouldRedact(const string& redactionType) {
> Can't we just strstr(FLAGS_redact, "flag")?
Done


http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/flags.h
File src/kudu/util/flags.h:

Line 68: bool ShouldRedact(const std::string&);
> Nit: missing parameter name.
Done


PS3, Line 70: // 'log' redaction type.
            : extern const char* const redactLog;
            : 
            : // 'flag' redaction type.
            : extern const char* const redactFlag;
> For callers outside flags.{cc,h} I think it'd be nicer to provide an enum f
Done


http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/logging.h
File src/kudu/util/logging.h:

Line 62: #define KUDU_SHOULD_REDACT() (kudu::ShouldRedact(kudu::redactLog) && kudu::tls_redact_user_data)
> this is a hot path, I think we should be more performance-conscious, and ex
Can you specify a bit more on how to do that? How would the boolean flag be notified of the value of redaction flag is changed?


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6112/11/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

PS11, Line 145: enabledRedactTypes
> Nit: this should be enabled_redact_types.
Ah... thanks for catching it again. :P


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 112: DEFINE_string(redact, "log,flag",
Please use a gflag validator (google::RegisterFlagValidator) to validate the value of the flag at runtime.


Line 113:               "Sensitive information that needs to be redacted (comma-separated "
How about: "Comma-separated list of redactions. Supported redactions are 'flag' and 'log'. If 'flag' is specified, the values of flags tagged as sensitive will be redacted whenever server configuration is emitted. If 'log' is specified, row data will be redacted from log and error messages.


Line 488: bool ShouldRedact(const string& redactionType) {
Can't we just strstr(FLAGS_redact, "flag")?

If/when we have redaction types that would yield false positives, we can adjust.


http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/flags.h
File src/kudu/util/flags.h:

Line 68: bool ShouldRedact(const std::string&);
Nit: missing parameter name.


PS3, Line 70: // 'log' redaction type.
            : extern const char* const redactLog;
            : 
            : // 'flag' redaction type.
            : extern const char* const redactFlag;
For callers outside flags.{cc,h} I think it'd be nicer to provide an enum for redaction types, and to pass in the enum value into ShouldRedact().

Internally ShouldRedact() can map from the enum into a string.


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#4).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with 'log'
and 'flag', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 113 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#8).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with
'all', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 118 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/8
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 17: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has uploaded a new patch set (#2).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with 'log'
and 'flag', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 61 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#7).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with 'log'
and 'flag', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 121 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 7:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

PS7, Line 130:   auto iter = RedactionUnit::redactionMap.find(value);
             :   if (iter != RedactionUnit::redactionMap.end()) {
             :     return true;
             :   }
You can use some of the handy functions from gutil/map-util.h for this, like ContainsKey().

But, given my other feedback, I think you should just do an if/elseif/else statement to figure out whether the values are safe.

Bear in mind that you should still parse the comma-separated list for entries. You may also want to disallow "all,foo" (because if all is there, it should be there on its own).


PS7, Line 140: static bool dummy [] = {
             :   google::RegisterFlagValidator(&FLAGS_umask, &ValidateUmask),
             :   google::RegisterFlagValidator(&FLAGS_redact, &ValidateRedact)
             : };
If you use the DEFINE_validator() macro, you won't need to worry about saving the result.


PS7, Line 509:   const char* redactFlags = FLAGS_redact.c_str();
             :   bool retVal;
Style: we use underscores when naming local variables, not camel case. Only constants and static stuff gets the kVarName treatment.


http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/flags.h
File src/kudu/util/flags.h:

PS7, Line 73: struct RedactionUnit {
            :   enum Type {
            :     kRedactLog,
            :     kRedactFlag,
            :     kRedactAll
            :   };
            :   static const char* Name(Type redactionType);
            :   static std::unordered_map<std::string, RedactionUnit::Type> redactionMap;
            : };
This was more complicated than I anticipated, so let's scale it back. Maybe two functions would work? Something like this:

  bool ShouldRedactLog();
  bool ShouldRedactFlag();

Then just call each one from wherever it needs to be called. Then there's no conversion between enums and c-strings and it should be a fair bit simpler.


http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/logging.h
File src/kudu/util/logging.h:

Line 62: // Evaluates to 'true' if the caller should redact any user data in the current scope.
> Can you specify a bit more on how to do that? How would the boolean flag be
The value cannot be changed; the redaction gflag isn't tagged as 'runtime'. So you'd just need to parse the main redaction flag at runtime (perhaps in the gflag validator, though maybe it's poor form for validators to have side effects) and store the result in a separate, global boolean.

On second read, I do see that the original 'log_redact_user_data' flag was tagged as 'runtime'; I don't think it's a big deal to lose that feature.


http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/logging.h
File src/kudu/util/logging.h:

Line 32: using kudu::ShouldRedact;
> warning: using decl 'ShouldRedact' is unused [misc-unused-using-decls]
These are legit; you shouldn't put "using" statements in headers.


Line 58:       kudu::ScopedDisableRedaction s;                      \
Nit: restore this indentation?


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 3:

Would be nice to use a case-insensitive comparison (boost::iequals).  Would be nice to provide an 'all' option for redacting all types.

-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 8:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

PS7, Line 130:     kudu::should_redact_log = false;
             :     return true;
             :   }
             : 
> You can use some of the handy functions from gutil/map-util.h for this, lik
Done


PS7, Line 140:     return true;
             :   }
             : 
             :   
> If you use the DEFINE_validator() macro, you won't need to worry about savi
Done


PS7, Line 509:     retval += f.name;
             :     retval += 
> Style: we use underscores when naming local variables, not camel case. Only
Done


http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/flags.h
File src/kudu/util/flags.h:

PS7, Line 73: 
            : 
            : 
            : 
            : 
            : 
            : 
            : 
            : 
> This was more complicated than I anticipated, so let's scale it back. Maybe
Done


http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/logging.h
File src/kudu/util/logging.h:

Line 62: #define KUDU_SHOULD_REDACT() (kudu::should_redact_log && kudu::tls_redact_user_data)
> The value cannot be changed; the redaction gflag isn't tagged as 'runtime'.
Done


http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/logging.h
File src/kudu/util/logging.h:

Line 32: ////////////////////////////////////////////////////////////////////////////////
> warning: using declarations in the global namespace in headers are prohibit
Done


Line 32: ////////////////////////////////////////////////////////////////////////////////
> These are legit; you shouldn't put "using" statements in headers.
Done


Line 32: ////////////////////////////////////////////////////////////////////////////////
> warning: using decl 'ShouldRedact' is unused [misc-unused-using-decls]
Done


Line 33: // Redaction support
> warning: using decl 'RedactionUnit' is unused [misc-unused-using-decls]
Done


Line 33: // Redaction support
> warning: using declarations in the global namespace in headers are prohibit
Done


Line 58: 
> Nit: restore this indentation?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#15).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with
'all', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
17 files changed, 100 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/15
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with
'all', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Reviewed-on: http://gerrit.cloudera.org:8080/6112
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
17 files changed, 100 insertions(+), 52 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#5).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with 'log'
and 'flag', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 113 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6112/9/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 55: bool should_redact_flag;
Should be prefixed with g_.


http://gerrit.cloudera.org:8080/#/c/6112/9/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:

Line 80: bool g_should_redact_log;
Could we define this alongside should_redact_flag and expose them in the same way, to be consistent?


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6112

to look at the new patch set (#12).

Change subject: Combine redaction flag for log and flags into one.
......................................................................

Combine redaction flag for log and flags into one.

This adds a new flag --redact to replace --redact_sensitive_flags
and --log_redact_user_data. By default, --redact is enabled with
'all', which means both log and flag redaction is on.

Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
---
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/types-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/test_util.cc
16 files changed, 98 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/6112/12
-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

PS3, Line 115: flags tagged as 'sensitive'will
this is somewhat uninterpretable to users, since they don't really know about flag tags.

I think better to say something like "configuration flags which may include sensitive data".


Line 118:               "redacted.");
can we also add an "all" or "*" mode?


PS3, Line 275: redactLog
style: REDACT_LOG or kRedactLog or something


http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/logging.h
File src/kudu/util/logging.h:

Line 62: #define KUDU_SHOULD_REDACT() (kudu::ShouldRedact(kudu::redactLog) && kudu::tls_redact_user_data)
this is a hot path, I think we should be more performance-conscious, and extract a global boolean here early during startup, rather than doing a string search each time.


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6112/9/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 55: bool should_redact_flag;
> Should be prefixed with g_.
Done


http://gerrit.cloudera.org:8080/#/c/6112/9/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:

Line 80: bool g_should_redact_log;
> Could we define this alongside should_redact_flag and expose them in the sa
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6112/10/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 327: 
> Sorry, I confused myself. What I meant was, can we declare g_should_redact_
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Combine redaction flag for log and flags into one.

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Combine redaction flag for log and flags into one.
......................................................................


Patch Set 8:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:

Line 1087:   gflags::SetCommandLineOption("redact", "log");
So this reruns the validator? Is that how the log boolean gets reset?

On calls like this we should ASSERT that the result is not an empty string. Empty strings are a sign that the function failed.


http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 150:   for (iter = enabledRedactTypes.begin(); iter != enabledRedactTypes.end(); ++iter) {
You can use an enhanced for loop:

  for (const auto&s : enabledRedactTypes) {
    if (s.compare("LOG") == 0) {
      ...
    }
  }


Line 165:     kudu::should_redact_log = enabled_redact_log;
Why don't we store the result of FLAG redaction in a global variable too? Then we don't need ShouldRedact() anymore, and ShouldRedactFlag() becomes much simpler.


http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/util/flags.h
File src/kudu/util/flags.h:

Line 68: bool ShouldRedactLog();
Why do we still need this? Either logging.h should call this and should_redact_log should be hidden in flags.cc, or we don't need this function.


http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:

Line 80: bool should_redact_log = true;
Does this actually need a default value, since it's always initialized via the gflag validator?


http://gerrit.cloudera.org:8080/#/c/6112/8/src/kudu/util/logging.h
File src/kudu/util/logging.h:

Line 86: extern bool should_redact_log;
Nit: we generally prefix global variables with g_.


-- 
To view, visit http://gerrit.cloudera.org:8080/6112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes