Adar Dembo has posted comments on this change. Change subject: KUDU-1353: remove per-tablet replica locations cache ......................................................................
Patch Set 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/2887/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 1636: // Note that we leave out replicas who live in tablet servers who have not heartbeated to > this comment needs updating Done Line 1651: VLOG(2) << "Resetting replicas for tablet " << final_report->tablet_id() > update here Done Line 1658: RETURN_NOT_OK(ResetTabletReplicasFromReportedConfig(*final_report, tablet, > I think this function should be renamed -- something like HandleRaftConfigC Done Line 1811: LOG(WARNING) << err_msg; > would this end up double-logged? Yeah it would; The call chain leading to this function already logs failures. Will remove this and below too. Line 1813: const ConsensusStatePB &cstate = l.data().pb.committed_consensus_state(); > nit: & with type name Done Line 1814: for (const auto &peer : cstate.config().peers()) { > instead of this loop, can you just use cstate.leader_uuid() here directly? Done Line 2508: if (!master_->ts_manager()->LookupTSByUUID(peer.permanent_uuid(), &ts_desc)) { > if you changed the TSPicker interface so that it was supposed to return a U This was a great observation; I was able to remove quite a few lookups. Line 2975: replica_pb->set_role(GetConsensusRole(peer.permanent_uuid(), cstate)); > there's some TODO worth adding here that this is O(n^2) because each GetCon Will mention. http://gerrit.cloudera.org:8080/#/c/2887/1/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: Line 134: // Lock protecting the below mutable fields. > which fields is this now? It's still the fields "below": last_update_time_ and reported_schema_version_. Line 138: // The last time the replica locations were updated. > this needs to be updated. is the lock still necessary? Strictly speaking no, since all the accesses come from the BgTasks thread. But the usage may evolve, and we still need the lock for reported_schema_version_ (I wanted to convert it to an std::atomic but it won't work; there's no atomic "compare and set if greater" primitive). But will rename the field since it's not really "last_update" anymore. http://gerrit.cloudera.org:8080/#/c/2887/1/src/kudu/master/master-path-handlers.cc File src/kudu/master/master-path-handlers.cc: Line 183: LOG(WARNING) << Substitute("Could not find TS with UUID $0", > i think it's better to ensure that _some_ kind of entry shows up in the res OK. Line 188: make_pair(ts_desc.get(), > make_pair isn't necessary here with emplace_back Done http://gerrit.cloudera.org:8080/#/c/2887/1/src/kudu/master/master-path-handlers.h File src/kudu/master/master-path-handlers.h: Line 67: <TSDescriptor*, consensus::RaftPeerPB::Role>>& locations, > I find this wrapping kind of weird. what if you just wrap before 'const'? Yeah, that'll work. http://gerrit.cloudera.org:8080/#/c/2887/1/src/kudu/master/master.proto File src/kudu/master/master.proto: Line 299: // DEPRECATED > maybe rename the field to DEPRECATED_stale? Sure, I can do that. -- To view, visit http://gerrit.cloudera.org:8080/2887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6376b5307f75f5d505b33a5ff4262da619cd1d8d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
