[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