[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. [util] Add ParseStringsWithScheme in net_util This commit adds a new util to allow parsing address with scheme, e.g. '://:/' into a : pair. Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Reviewed-on: http://gerrit.cloudera.org:8080/11843 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h 3 files changed, 104 insertions(+), 6 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified Alexey Serbin: Looks good to me, but someone else must approve -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 18:24:52 + Gerrit-HasComments: No
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@153 PS1, Line 153: > Hmm, I did not see how since StripPrefixString or StripSuffixString are rel Ah, yeah I saw what you mean; since the entirety of the scheme and path aren't known up front, you can't use those functions to find them. -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 17:13:20 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@110 PS2, Line 110: :1234 > What's wrong with this port? 'abc:1234/path' does not have scheme so valid format would be :, which '1234/path' is considered as the port number. http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@114 PS2, Line 114: :12 > And this one? Updated. http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@122 PS2, Line 122: ://scheme > Should this be "invalid scheme" or something? Isn't it the scheme that's in Hmm, makes sense. Updated. -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 17:02:19 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11843 to look at the new patch set (#3). Change subject: [util] Add ParseStringsWithScheme in net_util .. [util] Add ParseStringsWithScheme in net_util This commit adds a new util to allow parsing address with scheme, e.g. '://:/' into a : pair. Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 --- M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h 3 files changed, 104 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/11843/3 -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@110 PS2, Line 110: :1234 What's wrong with this port? http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@114 PS2, Line 114: :12 And this one? http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@122 PS2, Line 122: ://scheme Should this be "invalid scheme" or something? Isn't it the scheme that's invalid (since it's empty)? -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 06:25:06 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11843 to look at the new patch set (#2). Change subject: [util] Add ParseStringsWithScheme in net_util .. [util] Add ParseStringsWithScheme in net_util This commit adds a new util to allow parsing address with scheme, e.g. '://:/' into a : pair. Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 --- M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h 3 files changed, 98 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/11843/2 -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11843 )
Change subject: [util] Add ParseStringsWithScheme in net_util
..
Patch Set 1:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@46
PS1, Line 46: be
> nit: drop
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@46
PS1, Line 46: .
> nit: a comma (,) instead of a period (.)?
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@46
PS1, Line 46: // Similar to above but allow the address to be have scheme and
path. e.g.
> Should mention that the scheme and path are ignored.
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@49
PS1, Line 49: host
> Maybe, add a note that component cannot be in IPv6 address notation.
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@76
PS1, Line 76: // Similar to above but allow the addresses to be have scheme
and path.
> Same.
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@76
PS1, Line 76: be
> nit: drop
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@149
PS1, Line 149: Status HostPort::ParseStringWithScheme(const std::string &str,
uint16_t default_port) {
> No std:: prefix. Also const string&
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@150
PS1, Line 150: string uri(str);
> Nit: maybe name this "str_copy" so it's more clear that it's a copy?
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@153
PS1, Line 153:
> Maybe you can simplify some of this with gutil functions like StripPrefixSt
Hmm, I did not see how since StripPrefixString or StripSuffixString are relying
on exact matching?
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@217
PS1, Line 217: vector addr_strings = strings::Split(comma_sep_addrs,
",", strings::SkipEmpty());
> Once you've got this, you can reserve the appropriate capacity in res.
Done
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@221
PS1, Line 221: res->push_back(host_port);
> Nit: emplace_back for new code.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11843
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0
Gerrit-Change-Number: 11843
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 01 Nov 2018 05:10:35 +
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@46 PS1, Line 46: be nit: drop http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@46 PS1, Line 46: . nit: a comma (,) instead of a period (.)? http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@49 PS1, Line 49: host Maybe, add a note that component cannot be in IPv6 address notation. http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@76 PS1, Line 76: be nit: drop -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 04:25:23 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11843 )
Change subject: [util] Add ParseStringsWithScheme in net_util
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@46
PS1, Line 46: // Similar to above but allow the address to be have scheme and
path. e.g.
Should mention that the scheme and path are ignored.
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@76
PS1, Line 76: // Similar to above but allow the addresses to be have scheme
and path.
Same.
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@149
PS1, Line 149: Status HostPort::ParseStringWithScheme(const std::string &str,
uint16_t default_port) {
No std:: prefix. Also const string&
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@150
PS1, Line 150: string uri(str);
Nit: maybe name this "str_copy" so it's more clear that it's a copy?
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@153
PS1, Line 153:
Maybe you can simplify some of this with gutil functions like
StripPrefixString, StripSuffixString, etc?
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@217
PS1, Line 217: vector addr_strings = strings::Split(comma_sep_addrs,
",", strings::SkipEmpty());
Once you've got this, you can reserve the appropriate capacity in res.
http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@221
PS1, Line 221: res->push_back(host_port);
Nit: emplace_back for new code.
--
To view, visit http://gerrit.cloudera.org:8080/11843
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0
Gerrit-Change-Number: 11843
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 01 Nov 2018 04:14:17 +
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11843 Change subject: [util] Add ParseStringsWithScheme in net_util .. [util] Add ParseStringsWithScheme in net_util This commit adds a new util to allow parsing address with scheme, e.g. '://:/' into a : pair. Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 --- M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h 3 files changed, 89 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/11843/1 -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao
