Alexey Serbin 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) Took a quick look and decided to post a couple of questions before reviewing the rest of the patch. 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? 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? 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) this method resolves just one of tserver's PRC end-points 2) if the end-point has multiple DNS names, it outputs just one >From that perspective, could it be the case when a tablet server a) doesn't get the desired/expected location b) moves from one location to another just because of DNS round-robins multiple names ? Also, just another point to think about the design a bit: should the location script be able to take a) just IP address b) multiple IP addresses c) multiple DNS names ? http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@233 PS3, Line 233: location nit: std::move(location) ? -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 06 Aug 2018 17:56:28 +0000 Gerrit-HasComments: Yes
