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 2018/10/18 05:47:23 UTC

[kudu-CR] [sentry] improve SentryAction 

Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11720


Change subject: [sentry] improve SentryAction
......................................................................

[sentry] improve SentryAction

A follow-up to Adar's suggestions in
https://gerrit.cloudera.org/#/c/11656/.

Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
---
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
3 files changed, 42 insertions(+), 51 deletions(-)



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

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

[kudu-CR] [sentry] improve SentryAction

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 22 Oct 2018 20:21:37 +0000
Gerrit-HasComments: No

[kudu-CR] [sentry] improve SentryAction

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/11720

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

Change subject: [sentry] improve SentryAction
......................................................................

[sentry] improve SentryAction

A follow-up to Adar's suggestions in
https://gerrit.cloudera.org/#/c/11656/.

Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
---
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
3 files changed, 42 insertions(+), 52 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [sentry] improve SentryAction

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

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

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

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

Change subject: [sentry] improve SentryAction
......................................................................

[sentry] improve SentryAction

A follow-up to Adar's suggestions in
https://gerrit.cloudera.org/#/c/11656/.

Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
---
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
3 files changed, 68 insertions(+), 40 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [sentry] improve SentryAction

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h@66
PS2, Line 66:   static Status FromString(const std::string& str, Action* action);
I don't understand the second parameter here; I would have expected it to be of type SentryAction, but without a default constructor you can't construct an "empty" SentryAction in which FromString can write the result.

Is this useful in this form?


http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h@77
PS2, Line 77:   SentryAction() = delete;
Why is this necessary?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 18 Oct 2018 18:40:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h@66
PS2, Line 66:   static Status FromString(const std::string& str, Action* action);
> I don't understand the second parameter here; I would have expected it to b
I suggested adding the UNINITIALIZED enum so a default constructor could exist without having undefined behavior Imply()ing against a default-constructed SentryAction.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 18 Oct 2018 18:44:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 5: Verified+1

Flaky unrelated AdminCliTest.TestSimultaneousLeaderTransferAndAbruptStepdown test.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 22 Oct 2018 20:03:09 +0000
Gerrit-HasComments: No

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h@66
PS2, Line 66:   Action action() const {
> On second thought, my suggestion (to remove UNINITIALIZED and the default c
Done


http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h@77
PS2, Line 77:   //
> Why is this necessary?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Oct 2018 19:42:23 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed a vote on this change.

Change subject: [sentry] improve SentryAction
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/11720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [sentry] improve SentryAction

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 4: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11720/4/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/4/src/kudu/sentry/sentry_action.h@36
PS4, Line 36: enum class Action {
You can still make this nested inside SentryAction if you want; I don't think either ActionToString() or operator<< require it to be a stand-alone class.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Oct 2018 21:37:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed a vote on this change.

Change subject: [sentry] improve SentryAction
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/11720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................

[sentry] improve SentryAction

A follow-up to Adar's suggestions in
https://gerrit.cloudera.org/#/c/11656/.

Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Reviewed-on: http://gerrit.cloudera.org:8080/11720
Tested-by: Hao Hao <ha...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
3 files changed, 68 insertions(+), 40 deletions(-)

Approvals:
  Hao Hao: Verified
  Adar Dembo: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 2: Verified+1

Unrelated flaky failure AdminCliTest.TestSimultaneousLeaderTransferAndAbruptStepdown


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 18 Oct 2018 17:11:03 +0000
Gerrit-HasComments: No

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11720/3/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/3/src/kudu/sentry/sentry_action.h@48
PS3, Line 48: };
> Hmm, this was probably better as an enum class.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:59:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11720/3/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/3/src/kudu/sentry/sentry_action.h@48
PS3, Line 48:   enum Action {
Hmm, this was probably better as an enum class.

If the issue is logging the value, you'll need to write some boilerplate code. Check out AuthenticationType in rpc/negotiation.{cc,h}:

  enum class AuthenticationType {
    INVALID,
    SASL,
    TOKEN,
    CERTIFICATE,
  };
  const char* AuthenticationTypeToString(AuthenticationType t);

  std::ostream& operator<<(std::ostream& o, AuthenticationType authentication_type);



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Oct 2018 19:55:47 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

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

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

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

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

Change subject: [sentry] improve SentryAction
......................................................................

[sentry] improve SentryAction

A follow-up to Adar's suggestions in
https://gerrit.cloudera.org/#/c/11656/.

Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
---
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
3 files changed, 99 insertions(+), 72 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [sentry] improve SentryAction

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h@66
PS2, Line 66:   static Status FromString(const std::string& str, Action* action);
> Yes, Andrew suggested to add UNINITIALIZED enum to avoid undefined behavior
On second thought, my suggestion (to remove UNINITIALIZED and the default constructor) will only work if FromString() is more frustrating to use, either in the way that you've done here, or by forcing the second parameter SentryAction to be wrapped in a unique_ptr.

Neither of those solutions are particularly elegant, so I think you should ignore my suggestion, restore UNINITIALIZED, and restore the default constructor. Then the second parameter can be a SentryAction* (though I still think it's clearer for FromString to be a static factory method of that kind than a mutator).



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Oct 2018 16:49:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11720/4/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/4/src/kudu/sentry/sentry_action.h@36
PS4, Line 36: // One action can imply another following rules defined in Imply().
> You can still make this nested inside SentryAction if you want; I don't thi
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 22 Oct 2018 18:40:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )

Change subject: [sentry] improve SentryAction
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h@66
PS2, Line 66:   static Status FromString(const std::string& str, Action* action);
> I suggested adding the UNINITIALIZED enum so a default constructor could ex
Yes, Andrew suggested to add UNINITIALIZED enum to avoid undefined behavior of Imply() with default constructor https://gerrit.cloudera.org/#/c/11656/3/src/kudu/sentry/sentry_action-test.cc@95. And my understanding is you suggested to not add a new enum but explicitly delete the default constructor to avoid the UB as https://gerrit.cloudera.org/#/c/11656/6/src/kudu/sentry/sentry_action.h@46? Maybe my understanding is wrong?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Oct 2018 16:37:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] improve SentryAction

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

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

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

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

Change subject: [sentry] improve SentryAction
......................................................................

[sentry] improve SentryAction

A follow-up to Adar's suggestions in
https://gerrit.cloudera.org/#/c/11656/.

Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
---
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
3 files changed, 42 insertions(+), 41 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f
Gerrit-Change-Number: 11720
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)