You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/11/08 19:52:25 UTC
[kudu-CR] macOS: fix Kerberos tests where possible
Hello Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5005
to review the following change.
Change subject: macOS: fix Kerberos tests where possible
......................................................................
macOS: fix Kerberos tests where possible
Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/rpc/sasl_rpc-test.cc
2 files changed, 46 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5005/1
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: macOS: fix Kerberos tests where possible
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: macOS: fix Kerberos tests where possible
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5005/3/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
PS3, Line 50: // Since we don't have any way to explicitly figure out the version, we just
: // look for this random macro which was added in 1.11 (the same version in which
: // the above bug was fixed).
> If we end up doing more version-specific work for krb5, it would be best to
This diff is the result of the rebase.
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: macOS: fix Kerberos tests where possible
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5005/1//COMMIT_MSG
Commit Message:
PS1, Line 7: where possible
> I think you missed this.
Done
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5005
to look at the new patch set (#4).
Change subject: macOS: fix Kerberos tests where possible
......................................................................
macOS: fix Kerberos tests where possible
This commit fixes Kerberos tests on OS X where possible, or excludes
them from the build. The issues that can't be worked around only appear
to affect tests of invalid authentication, so hopefully they will be
limited to the Kerberos-specific tests and will not require further
workarounds for more general purpose tests when security is enabled.
Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/rpc/sasl_rpc-test.cc
2 files changed, 46 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5005/4
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: macOS: fix Kerberos tests where possible
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5005/1//COMMIT_MSG
Commit Message:
PS1, Line 7: where possible
Can you elaborate on this in the rest of the commit message? What is the state of the Kerberos-tests-in-Kudu world following this patch?
http://gerrit.cloudera.org:8080/#/c/5005/1/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
Line 262: CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/));
Won't setenv() leak into the run of other tests in the same file? If so, would you mind creating an RAII-style enabler/disabler for safe setenv() use?
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5005
to look at the new patch set (#3).
Change subject: macOS: fix Kerberos tests where possible
......................................................................
macOS: fix Kerberos tests where possible
Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/rpc/sasl_rpc-test.cc
2 files changed, 46 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5005/3
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: macOS: fix Kerberos tests where possible
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5005/1//COMMIT_MSG
Commit Message:
PS1, Line 7: where possible
> Can you elaborate on this in the rest of the commit message? What is the st
I think you missed this.
http://gerrit.cloudera.org:8080/#/c/5005/3/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
PS3, Line 50: public:
: virtual void SetUp() OVERRIDE {
: RpcTestBase::SetUp();
If we end up doing more version-specific work for krb5, it would be best to move this sort of thing into cmake. But, since this is the only such place right now, don't bother.
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: macOS: fix Kerberos tests where possible
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5005/1/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
Line 262: CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/));
> Won't setenv() leak into the run of other tests in the same file? If so, wo
KuduTest::SetUp() overrides the krb5 environment back to invalid paths so that each test case is self-contained (sort of like how flagsaver preserves gflag values)
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] macOS: fix Kerberos tests where possible
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: macOS: fix Kerberos tests where possible
......................................................................
macOS: fix Kerberos tests where possible
This commit fixes Kerberos tests on OS X where possible, or excludes
them from the build. The issues that can't be worked around only appear
to affect tests of invalid authentication, so hopefully they will be
limited to the Kerberos-specific tests and will not require further
workarounds for more general purpose tests when security is enabled.
Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Reviewed-on: http://gerrit.cloudera.org:8080/5005
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/rpc/sasl_rpc-test.cc
2 files changed, 46 insertions(+), 27 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I569a5c94b4aeba34b03690906fa1e356b55d9688
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>