Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/20276 )
Change subject: IMPALA-12321: Fix the race condition for updating active catalogd ...................................................................... Patch Set 2: (11 comments) Looks generally good http://gerrit.cloudera.org:8080/#/c/20276/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20276/2//COMMIT_MSG@7 PS2, Line 7: for Nit: "when" http://gerrit.cloudera.org:8080/#/c/20276/2//COMMIT_MSG@20 PS2, Line 20: added I think it is already there? So nit: "already" http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/catalog/catalog-server.h File be/src/catalog/catalog-server.h: http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/catalog/catalog-server.h@170 PS2, Line 170: is Nit: "was" http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/runtime/exec-env.h@211 PS2, Line 211: last_active_catalogd_seq_ I think this should be 'last_active_catalogd_version_' ? http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/runtime/exec-env.h@330 PS2, Line 330: is Nit: "was" http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/runtime/exec-env.cc@671 PS2, Line 671: if (is_registration_reply) { It's unfortunate that this code is duplicated in exec-env.cc and catalog-server.cc Could we have a class that contained last_update_for_registration_ last_active_catalogd_version_ and a method that contained the common code? Then each of CatalogServer and ExecEnv could hold an instance of the class, and both versions of UpdateActiveCatalogd would call the new method. I agree that some duplicated code is OK so this is just a suggestion, please ignore if you wish. http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/statestore/statestore-subscriber.h File be/src/statestore/statestore-subscriber.h: http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/statestore/statestore-subscriber.h@124 PS2, Line 124: the Nit: "this" http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/statestore/statestore-subscriber.h@124 PS2, Line 124: be Nit: "been" http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/statestore/statestore-subscriber.cc@287 PS2, Line 287: Skipped Nit: "Skip" http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/statestore/statestore-subscriber.cc@303 PS2, Line 303: Skipped Nit: "Skip" http://gerrit.cloudera.org:8080/#/c/20276/2/tests/custom_cluster/test_catalogd_ha.py File tests/custom_cluster/test_catalogd_ha.py: http://gerrit.cloudera.org:8080/#/c/20276/2/tests/custom_cluster/test_catalogd_ha.py@366 PS2, Line 366: assert(catalogd_service_1.get_metric_value("catalog-server.active-status") For completeness do we also need assert(catalogd_service_1.get_metric_value("catalog-server.active-status") || catalogd_service_2.get_metric_value("catalog-server.active-status")) (to check that one is active)? -- To view, visit http://gerrit.cloudera.org:8080/20276 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie49947e563d43c59bdd476b28c35be69848ae12a Gerrit-Change-Number: 20276 Gerrit-PatchSet: 2 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 28 Jul 2023 01:10:54 +0000 Gerrit-HasComments: Yes
