[kudu-CR] Forward compat for changing TSRegistrationPB
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 PercyGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Forward compat for changing TSRegistrationPB
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 PercyGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Forward compat for changing TSRegistrationPB
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 PercyGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Forward compat for changing TSRegistrationPB
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 PercyGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Forward 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 (#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 PercyGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Forward compat for changing TSRegistrationPB
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 PercyGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Forward compat for changing TSRegistrationPB
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 PercyGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[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 PercyGerrit-Reviewer: Todd Lipcon