Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/11115/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11115/3//COMMIT_MSG@21 PS3, Line 21: I also altered the DATA_FILES CMake function so that data files copied : to the build directory or to slaves by dist-test are executable as well : as readable. This was necessary for the new TSDescriptor test which : tests location assignment. > nit: does it make sense to separate this into its own patch? To me, not really. It's an update to a comment and adding two words to one line. A separate patch will just be that tiny change and a commit message saying "I need to do this for another patch". http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@176 PS3, Line 176: CHECK_EQ(instance.permanent_uuid(), permanent_uuid_); : : // TODO(KUDU-418): we don't currently support changing RPC addresses since the : // host/port is stored persistently in each tablet's metadata. : if (registration_ && : !HostPortPBsEqual(registration_->rpc_addresses(), registration.rpc_addresses())) { : string msg = strings::Substitute( : "Tablet server $0 is attempting to re-register with a different host/port. " : "This is not currently supported. Old: {$1} New: {$2}", : instance.permanent_uuid(), : SecureShortDebugString(*registration_), : SecureShortDebugString(registration)); : LOG(ERROR) << msg; : return Status::InvalidArgument(msg); : } : : if (registration.rpc_addresses().empty()) { : return Status::InvalidArgument( : "invalid registration: must have at least one RPC address", : SecureShortDebugString(registration)); : } : : if (instance.instance_seqno() < latest_seqno_) { : return Status::AlreadyPresent( : strings::Substitute("Cannot register with sequence number $0:" : " Already have a registration from sequence number $1", : instance.instance_seqno(), : latest_seqno_)); : } else if (instance.instance_seqno() == latest_seqno_) { : // It's possible that the TS registered, but our response back to it : // got lost, so it's trying to register again with the same sequence : // number. That's fine. : LOG(INFO) << "Processing retry of TS registration from " << SecureShortDebugString(instance); : } : : latest_seqno_ = instance.instance_seqno(); : registration_.reset(new ServerRegistrationPB(registration)); : ts_admin_proxy_.reset(); : consensus_proxy_.reset(); > nit: maybe, extract this into a method? RegisterUnlocked? Done http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@224 PS3, Line 224: ResolveSockaddr > I have some concerns about using thing method. 1) and 2) are valid points, but they don't apply strictly to this patch. They apply to Kudu in general. We don't support multiple RPC addresses for a node. I think doing so here would be surprising if the support is lacking elsewhere. Why assign a location based on an address that Kudu can never use? We spoke about the second issue a bit. The script should accept IP or hostname, as the address may resolve into either. For this first iteration, it doesn't seem necessary to resolve multiple ip/hostnames into locations. If we were to do that, we ought to use it, and to use it we'd need to devise some kind of batch tablet server registration process or a batch location assignment process for clients. http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@233 PS3, Line 233: location > nit: std::move(location) ? Done -- To view, visit http://gerrit.cloudera.org:8080/11115 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878 Gerrit-Change-Number: 11115 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 06 Aug 2018 18:23:48 +0000 Gerrit-HasComments: Yes
