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 2017/07/05 23:31:35 UTC
[kudu-CR] MiniKdc: move common logic into test-utils
Dan Burkert has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7356
Change subject: MiniKdc: move common logic into test-utils
......................................................................
MiniKdc: move common logic into test-utils
Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
---
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
4 files changed, 108 insertions(+), 100 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7356/1
--
To view, visit http://gerrit.cloudera.org:8080/7356
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
[kudu-CR] MiniKdc: move common logic into test-utils
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/7356
to look at the new patch set (#2).
Change subject: MiniKdc: move common logic into test-utils
......................................................................
MiniKdc: move common logic into test-utils
Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
---
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
4 files changed, 110 insertions(+), 100 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7356/2
--
To view, visit http://gerrit.cloudera.org:8080/7356
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] MiniKdc: move common logic into test-utils
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: MiniKdc: move common logic into test-utils
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7356/2/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS2, Line 368: vector<string> lines = strings::Split(lsof_out, "\n");
: int32_t p = -1;
: if (lines.size() != 3 ||
: lines[2].substr(0, 3) != "n*:" ||
: !safe_strto32(lines[2].substr(3), &p) ||
: p <= 0) {
: return Status::RuntimeError("unexpected lsof output", lsof_out)
> For the TCP connections, it might happen the process not only started liste
It's difficult to add the LISTEN flag, because it's not available for UDP sockets. I'm not too keen on adding a bunch of options to this method, because it seems to work pretty well right now, and if we need more from it we should probably rethink using lsof at all.
--
To view, visit http://gerrit.cloudera.org:8080/7356
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc: move common logic into test-utils
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: MiniKdc: move common logic into test-utils
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7356/2/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS2, Line 368: vector<string> lines = strings::Split(lsof_out, "\n");
: int32_t p = -1;
: if (lines.size() != 3 ||
: lines[2].substr(0, 3) != "n*:" ||
: !safe_strto32(lines[2].substr(3), &p) ||
: p <= 0) {
: return Status::RuntimeError("unexpected lsof output", lsof_out)
For the TCP connections, it might happen the process not only started listening, but an incoming connection is accepted already. This this case this check might unexpectedly return RuntimeError.
Consider adding '-a -s TCP:BOUND' or '-a -s TCP:LISTEN' to the list of lsof options.
Also, if the process listens to many ports, this would also return RuntimeError. Is it possible to change the signature of the function and this parser code to accommodate the case when a processes starts listening on multiple ports?
--
To view, visit http://gerrit.cloudera.org:8080/7356
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc: move common logic into test-utils
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: MiniKdc: move common logic into test-utils
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7356/2/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS2, Line 368: vector<string> lines = strings::Split(lsof_out, "\n");
: int32_t p = -1;
: if (lines.size() != 3 ||
: lines[2].substr(0, 3) != "n*:" ||
: !safe_strto32(lines[2].substr(3), &p) ||
: p <= 0) {
: return Status::RuntimeError("unexpected lsof output", lsof_out)
> It's difficult to add the LISTEN flag, because it's not available for UDP s
Well, it could be just an array (vector) of options to join using via '-a' flag, so for BindTcp it would be {"-i4TCP", "-sTCP:LISTEN"} and for BindUdp {"-i4UDP"}, while the WaitForBind() method could be refactored a bit to accept std::vector<string> as a parameter instead of the 'kind' string parameter.
But if you feel it's not worth it, it's not a problem -- it can be added later. If needed, we can modify it while introducing support for multiple ports bound/listened.
--
To view, visit http://gerrit.cloudera.org:8080/7356
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc: move common logic into test-utils
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: MiniKdc: move common logic into test-utils
......................................................................
MiniKdc: move common logic into test-utils
Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
Reviewed-on: http://gerrit.cloudera.org:8080/7356
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
4 files changed, 110 insertions(+), 100 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7356
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie3f50e315f3e7ea16b3c73f058248c3dbd47d72e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins