[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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;
: HostPortSet hostports;
Interesting, but why not just:
unordered_set
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 hps1(pb1.begin(), pb1.end());
unordered_set 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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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;
: 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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB
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 --- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
