Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10289 )
Change subject: IMPALA-6948: Delete catalog update topic entries upon catalog restart ...................................................................... Patch Set 3: (2 comments) looks good. one remaining question about tightening the protocol. http://gerrit.cloudera.org:8080/#/c/10289/1/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/10289/1/be/src/statestore/statestore.cc@665 PS1, Line 665: subscriber->SetLastTopicVersionProcessed(topic_it->first, update.from_version); > I see what you mean. Ideally, I want to keep that operation (deleting the t makes sense. however, if I were to test these code paths, I would add a case where from_version is set *and* we clear_topic_entries(). current code in the catalog (a subscriber) does not allow this combination afaict. main question from the standpoint of the protocol is whether we even want to consider this combination. if not, then lets document it with a dcheck. http://gerrit.cloudera.org:8080/#/c/10289/3/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/10289/3/be/src/statestore/statestore.cc@215 PS3, Line 215: - if all is correct, the metric won't be negative. however, what happens if it does go negative? -- To view, visit http://gerrit.cloudera.org:8080/10289 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74a8ade8e498ac35cb56d3775d2c67a86988d9b6 Gerrit-Change-Number: 10289 Gerrit-PatchSet: 3 Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Mon, 07 May 2018 19:22:53 +0000 Gerrit-HasComments: Yes
