Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20276 )
Change subject: IMPALA-12321: Fix the race condition when updating active catalogd ...................................................................... Patch Set 3: (13 comments) 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: whe > Nit: "when" fixed http://gerrit.cloudera.org:8080/#/c/20276/2//COMMIT_MSG@20 PS2, Line 20: alrea > I think it is already there? So nit: "already" fixed 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: > Nit: "was" fixed http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/catalog/catalog-server.cc@559 PS2, Line 559: catalog_update_cv_.Wait(unique_lock); > nit: I think it is useful to also print information about last_active_catal Fixed as suggested 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: order. > I think this should be 'last_active_catalogd_version_' ? fixed http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/runtime/exec-env.h@330 PS2, Line 330: > Nit: "was" fixed 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: int64 active_catalogd_version, const TCatalogRegistration& catalogd_registration) { > It's unfortunate that this code is duplicated in exec-env.cc and catalog-se Defined a new class ActiveCatalogVersionChecker. http://gerrit.cloudera.org:8080/#/c/20276/1/be/src/statestore/statestore-catalogd-mgr.cc File be/src/statestore/statestore-catalogd-mgr.cc: http://gerrit.cloudera.org:8080/#/c/20276/1/be/src/statestore/statestore-catalogd-mgr.cc@126 PS1, Line 126: DCHECK(num_registered_catalogd_ == 2) > nit: In that case, can we add explicit error message here that Statestore d Good suggestion. will reject registration for such case in following patch. 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: be > Nit: "been" Done http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/statestore/statestore-subscriber.h@124 PS2, Line 124: n t > Nit: "this" Done 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: Skip th > Nit: "Skip" fixed http://gerrit.cloudera.org:8080/#/c/20276/2/be/src/statestore/statestore-subscriber.cc@303 PS2, Line 303: Skip th > Nit: "Skip" fixed 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 If two boolean variables are not equal, one equals false and another equals true. -- 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: 3 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 06:30:34 +0000 Gerrit-HasComments: Yes
