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 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, ...)) and lets explicitly state that 0 means no updates have been sent. 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_ (I see value bytes is decreased elsewhere) decrement the gauges? (I didn't see these decreased anywhere even though value bytes can decrease) why should last_version_ not be reset? 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? 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 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. 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? similar question with the version. how can we ensure that this callback does not silently pass the test without checking the assertions on L546, 547? perhaps log and check that the log contains a line that proves the check was run? -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 04 May 2018 19:02:38 +0000 Gerrit-HasComments: Yes
