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/11/01 03:36:25 UTC
[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.
'<fs>://<host>:<port>/<path>' into a <host>:<port> 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 <ha...@cloudera.com>
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <host> 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<string> 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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 01 Nov 2018 05:10:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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.
'<fs>://<host>:<port>/<path>' into a <host>:<port> 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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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, 01 Nov 2018 06:25:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <host>:<port>, 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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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, 01 Nov 2018 17:02:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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<string> 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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 01 Nov 2018 04:14:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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.
'<fs>://<host>:<port>/<path>' into a <host>:<port> 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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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, 01 Nov 2018 17:13:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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, 01 Nov 2018 18:24:52 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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.
'<fs>://<host>:<port>/<path>' into a <host>:<port> pair.
Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0
Reviewed-on: http://gerrit.cloudera.org:8080/11843
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <host> 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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 01 Nov 2018 04:25:23 +0000
Gerrit-HasComments: Yes