Dimitris Tsirogiannis 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 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/10289/1/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/10289/1/be/src/catalog/catalog-server.cc@249 PS1, Line 249: last_sent_catalog_version_ > add to the header comments for this member its expected range ( [0, ...)) a Well that is not entirely true. See L237, upon statestore restart it asks for all the catalog objects. But setting last_sent_catalog_version_ to 0 doesn't mean that the catalog hasn't sent any other updates during its lifetime. 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@213 PS1, Line 213: topic_update_log_.clear(); > init the other members: total_key_size_bytes_, total_value_size_bytes_ Good point. I updated the metrics and the related fields. I don't think reseting last_version_ is a good idea. Topic versions are used by statestore to keep track of the topic updates that are sent to each subscriber. Reseting the last_version_ may result in complicated error scenarios that are hard to diagnose and debug. http://gerrit.cloudera.org:8080/#/c/10289/1/be/src/statestore/statestore.cc@665 PS1, Line 665: // cleared. > I didn't see that the case on L656 can happen when clearing. dcheck it? Hm, not following. What do you suggest we dcheck? http://gerrit.cloudera.org:8080/#/c/10289/1/tests/statestore/test_statestore.py File tests/statestore/test_statestore.py: http://gerrit.cloudera.org:8080/#/c/10289/1/tests/statestore/test_statestore.py@525 PS1, Line 525: sets the clear_topic_entries flag in a topic update message. """ > add the jira for context Done http://gerrit.cloudera.org:8080/#/c/10289/1/tests/statestore/test_statestore.py@535 PS1, Line 535: num_updates=1, : key_template= > keep the same order of args as on L531. Done http://gerrit.cloudera.org:8080/#/c/10289/1/tests/statestore/test_statestore.py@548 PS1, Line 548: > can some topic other than topic_name be called back here? The wait_for_update function will cause the subscriber to wait for a number of updates for a specific topic. So, I don't see how this could be skipped. -- 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: 1 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: Fri, 04 May 2018 21:20:38 +0000 Gerrit-HasComments: Yes
