Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/9123 )
Change subject: IMPALA-6437: separate AC/scheduler from catalog topic updates ...................................................................... Patch Set 11: (12 comments) http://gerrit.cloudera.org:8080/#/c/9123/6/be/src/statestore/statestore-subscriber.h File be/src/statestore/statestore-subscriber.h: http://gerrit.cloudera.org:8080/#/c/9123/6/be/src/statestore/statestore-subscriber.h@193 PS6, Line 193: update_lock http://gerrit.cloudera.org:8080/#/c/9123/11/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/9123/11/be/src/statestore/statestore.h@356 PS11, Line 356: GetSubscribedTopics This name is confusing. Maybe GetPriorityClass() http://gerrit.cloudera.org:8080/#/c/9123/10/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/9123/10/be/src/statestore/statestore.h@71 PS10, Line 71: the same update GatherTopicUpdates() will be called again and it's not the same update http://gerrit.cloudera.org:8080/#/c/9123/10/be/src/statestore/statestore.h@260 PS10, Line 260: to extra to http://gerrit.cloudera.org:8080/#/c/9123/10/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/9123/10/be/src/statestore/statestore.cc@161 PS10, Line 161: insert(make_pair emplace http://gerrit.cloudera.org:8080/#/c/9123/10/be/src/statestore/statestore.cc@174 PS10, Line 174: insert(make_pair emplace http://gerrit.cloudera.org:8080/#/c/9123/10/be/src/statestore/statestore.cc@285 PS10, Line 285: GetSubscribedTopics(topic.topic_name)->emplace(topic.topic_name, topic.is_transient) The copy/move-constructable requirement can be avoided with emplace(std::piecewise_construct, std::forward_as_tuple(topic.topic_name), std::forward_as_tuple(topic.is_transient)); This way the move constructor in atomics could be removed, which doesn't make much semantic sense and is prone to misuse. The same could be done to TopicMap and remove the unique_ptr boxing. http://gerrit.cloudera.org:8080/#/c/9123/10/be/src/statestore/statestore.cc@511 PS10, Line 511: scriber I immediately http://gerrit.cloudera.org:8080/#/c/9123/11/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/9123/11/be/src/statestore/statestore.cc@156 PS11, Line 156: const TTopicItem& item: entries I think entry and item mean the same thing and should be unified. http://gerrit.cloudera.org:8080/#/c/9123/11/be/src/statestore/statestore.cc@174 PS11, Line 174: insert(make_pai emplace http://gerrit.cloudera.org:8080/#/c/9123/11/be/src/statestore/statestore.cc@298 PS11, Line 298: == true remove http://gerrit.cloudera.org:8080/#/c/9123/11/be/src/statestore/statestore.cc@651 PS11, Line 651: : vector<TopicEntry::Version> entry_versions = topic->Put(update.topic_entries); : if (!subscriber->AddTransientEntries( : update.topic_name, update.topic_entries, entry_versions)) { : // Subscriber was unregistered - clean up the transient entries. : for (int i = 0; i < update.topic_entries.size(); ++i) { : topic->DeleteIfVersionsMatch(entry_versions[i], update.topic_entries[i].key); : } : } Why not just hold transient_entry_lock_ and check unregistered_, then put, instead of doing this put-then-revert? -- To view, visit http://gerrit.cloudera.org:8080/9123 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc49c2d0f2a5bfad822545616b8c62b4b95dc210 Gerrit-Change-Number: 9123 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Sat, 03 Feb 2018 17:58:22 +0000 Gerrit-HasComments: Yes
