You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2016/08/19 09:11:03 UTC
[kudu-CR] Forward compat for changing TSRegistrationPB
Hello Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4062
to review the following change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Forward compat for changing TSRegistrationPB
This should allow for adding fields to TSRegistrationPB
Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
---
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.h
2 files changed, 42 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/4062/1
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4062/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
Line 58: // Compares two repeated HostPortPB fields. Returns true if equal, false otherwise.
> This will return false if the two containers have the same contents, but ar
I think pbs are deterministic in this fashion (unlike JSON).
The proto3 library has some library code for various types of PB equality, but sadly we're still on proto2
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@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] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4062
to look at the new patch set (#6).
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Rolling-upgrades compat for changing TSRegistrationPB
This should allow for adding fields to TSRegistrationPB.
Only the host/port is checked.
Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
---
M src/kudu/master/master-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
4 files changed, 70 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/4062/6
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Patch Set 5:
Build Started http://104.196.14.100/job/kudu-gerrit/3062/
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Patch Set 6:
Build Started http://104.196.14.100/job/kudu-gerrit/3069/
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/2995/
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4062/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
Line 58: // Compares two repeated HostPortPB fields. Returns true if equal, false otherwise.
> I wasn't worried about non-determinism on the part of protobuf, but on the
oh, i see, yea that makes sense
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@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] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4062/4/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
PS4, Line 68: using HostPortSet = std::unordered_set<HostPort, HostPort::Hasher, HostPort::EqualityPredicate>;
: HostPortSet hostports;
Interesting, but why not just:
unordered_set<HostPort, HostPort::Hasher, HostPort::EqualityPredicate> hostports;
PS4, Line 70: for (int i = 0; i < pb1.size(); i++) {
: HostPort hp1;
: if (!HostPortFromPB(pb1.Get(i), &hp1).ok()) return false;
: hostports.insert(hp1);
: }
: for (int i = 0; i < pb1.size(); i++) {
: HostPort hp2;
: if (!HostPortFromPB(pb2.Get(i), &hp2).ok()) return false;
: if (!ContainsKey(hostports, hp2)) {
: return false;
: }
: }
You may be able to get away with unordered_sets of HostPortPB (maybe the generated code does the right thing):
unordered_set<HostPortPB> hps1(pb1.begin(), pb1.end());
unordered_set<HostPortPB> hps2(pb2.begin(), pb2.end());
return hps1 == hps2;
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4062
to look at the new patch set (#5).
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Rolling-upgrades compat for changing TSRegistrationPB
This should allow for adding fields to TSRegistrationPB.
Only the host/port is checked.
Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
---
M src/kudu/master/master-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
4 files changed, 72 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/4062/5
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4062
to look at the new patch set (#3).
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Forward compat for changing TSRegistrationPB
This should allow for adding fields to TSRegistrationPB
Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
---
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.h
2 files changed, 25 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/4062/3
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/2996/
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Patch Set 3:
(2 comments)
> How about a unit test showing a successful reregistration when
> something other than the addresses has changed?
Done
http://gerrit.cloudera.org:8080/#/c/4062/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
Line 58: // Compares two repeated HostPortPB fields. Returns true if equal, false otherwise.
> oh, i see, yea that makes sense
Done
http://gerrit.cloudera.org:8080/#/c/4062/3/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
Line 56: return port() == other.port() && host() == other.host();
> Any particular reason to compare the port before the host? I'd think the la
Comparing integers is cheaper.
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Rolling-upgrades compat for changing TSRegistrationPB
This should allow for adding fields to TSRegistrationPB.
Only the host/port is checked.
Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Reviewed-on: http://gerrit.cloudera.org:8080/4062
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/master/master-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
4 files changed, 70 insertions(+), 1 deletion(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Patch Set 3:
(2 comments)
How about a unit test showing a successful reregistration when something other than the addresses has changed?
http://gerrit.cloudera.org:8080/#/c/4062/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
Line 58: // Compares two repeated HostPortPB fields. Returns true if equal, false otherwise.
This will return false if the two containers have the same contents, but are out of order. Perhaps we should do a set comparison?
http://gerrit.cloudera.org:8080/#/c/4062/3/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
Line 56: return port() == other.port() && host() == other.host();
Any particular reason to compare the port before the host? I'd think the latter is more likely to be "different" and so should come first.
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@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] Forward compat for changing TSRegistrationPB
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2994/
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has uploaded a new patch set (#2).
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Forward compat for changing TSRegistrationPB
This should allow for adding fields to TSRegistrationPB
Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
---
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.h
2 files changed, 46 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/4062/2
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4062
to look at the new patch set (#4).
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Rolling-upgrades compat for changing TSRegistrationPB
This should allow for adding fields to TSRegistrationPB.
Only the host/port is checked.
Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
---
M src/kudu/master/master-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
4 files changed, 78 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/4062/4
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Forward compat for changing TSRegistrationPB
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Forward compat for changing TSRegistrationPB
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4062/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
Line 58: // Compares two repeated HostPortPB fields. Returns true if equal, false otherwise.
> I think pbs are deterministic in this fashion (unlike JSON).
I wasn't worried about non-determinism on the part of protobuf, but on the part of the sender. Suppose the sender stores the addresses in a hash-based container. Adding a new entry means the iteration order changes, at which point the iteration order of the PB repeated field here will be different too.
If the comparison done is a set comparison, it'll be protected from that.
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@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] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/3057/
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4062/5/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
PS5, Line 60: static bool HostPortPBsEqual(const ::google::protobuf::RepeatedPtrField<::kudu::HostPortPB>& pb1,
: const ::google::protobuf::RepeatedPtrField<::kudu::HostPortPB>& pb2)
> Not related to your change, but can you get away with HostPortPB instead of
Done
http://gerrit.cloudera.org:8080/#/c/4062/5/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
PS5, Line 55: bool Equals(const HostPort& other) const {
: return port_ == other.port_ && host_ == other.host_;
: }
> Since this is new, consider dropping it in favor of just operator==, which
Done
Line 59: bool operator==(const HostPort& other) const {
> Google style guide says const binary operators should be defined as free fu
Hmm, didn't know about that one. ODR requires me to rejigger a couple things. Done.
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4062/5/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
PS5, Line 60: static bool HostPortPBsEqual(const ::google::protobuf::RepeatedPtrField<::kudu::HostPortPB>& pb1,
: const ::google::protobuf::RepeatedPtrField<::kudu::HostPortPB>& pb2)
Not related to your change, but can you get away with HostPortPB instead of ::kudu::HostPortPB? I thought namespace resolution automatically checked parent namespaces.
http://gerrit.cloudera.org:8080/#/c/4062/5/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
PS5, Line 55: bool Equals(const HostPort& other) const {
: return port_ == other.port_ && host_ == other.host_;
: }
Since this is new, consider dropping it in favor of just operator==, which is pretty self-explanatory.
Line 59: bool operator==(const HostPort& other) const {
Google style guide says const binary operators should be defined as free functions, so that implicit constructors (if any) can apply to both operands.
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Rolling-upgrades compat for changing TSRegistrationPB
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4062/4/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
PS4, Line 68: using HostPortSet = std::unordered_set<HostPort, HostPort::Hasher, HostPort::EqualityPredicate>;
: HostPortSet hostports;
> Interesting, but why not just:
Ah, I was planning on having to refer to the type and ended up never having to. Removed.
PS4, Line 70: for (int i = 0; i < pb1.size(); i++) {
: HostPort hp1;
: if (!HostPortFromPB(pb1.Get(i), &hp1).ok()) return false;
: hostports.insert(hp1);
: }
: for (int i = 0; i < pb1.size(); i++) {
: HostPort hp2;
: if (!HostPortFromPB(pb2.Get(i), &hp2).ok()) return false;
: if (!ContainsKey(hostports, hp2)) {
: return false;
: }
: }
> You may be able to get away with unordered_sets of HostPortPB (maybe the ge
This would have been really nice, but it doesn't work. I think it's probably easy with proto3. I partially took your advice with the unordered_set equality thing though.
--
To view, visit http://gerrit.cloudera.org:8080/4062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes