Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20372 )
Change subject: IMPALA-12156: Support High Availability for Statestore ...................................................................... Patch Set 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/20372/11/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: http://gerrit.cloudera.org:8080/#/c/20372/11/be/src/statestore/statestore-subscriber.cc@178 PS11, Line 178: if (status.ok()) { We should probably log if status is not ok, right? Although it looks like we haven't done that previously anywhere. http://gerrit.cloudera.org:8080/#/c/20372/11/be/src/statestore/statestore-subscriber.cc@326 PS11, Line 326: // Both statestoreds are not ready. How does this get resolved? http://gerrit.cloudera.org:8080/#/c/20372/11/be/src/statestore/statestore-subscriber.cc@358 PS11, Line 358: if (statestore_->IsMatchingStatestoreId(statestore_id) || statestore2_ == nullptr) { Why do we need to worry about statestore2_ == nullptr here? http://gerrit.cloudera.org:8080/#/c/20372/11/be/src/statestore/statestore-subscriber.cc@410 PS11, Line 410: // statstoreds. nit: statestoreds http://gerrit.cloudera.org:8080/#/c/20372/11/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: http://gerrit.cloudera.org:8080/#/c/20372/11/common/thrift/StatestoreService.thrift@499 PS11, Line 499: // statstore should set itself as inactive if dst_statestore_active is set as true. nit: statestore http://gerrit.cloudera.org:8080/#/c/20372/11/common/thrift/StatestoreService.thrift@500 PS11, Line 500: // Otherwise, source statstore sets itself as active. nit: statestore http://gerrit.cloudera.org:8080/#/c/20372/11/tests/custom_cluster/test_statestored_ha.py File tests/custom_cluster/test_statestored_ha.py: http://gerrit.cloudera.org:8080/#/c/20372/11/tests/custom_cluster/test_statestored_ha.py@42 PS11, Line 42: as true. """ nit: extra spaces http://gerrit.cloudera.org:8080/#/c/20372/11/tests/custom_cluster/test_statestored_ha.py@48 PS11, Line 48: _, port = handle.getsockname() I don't see this used anywhere. -- To view, visit http://gerrit.cloudera.org:8080/20372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd2c814bbad5c04c1d50c2edaa5b910c82a6fd87 Gerrit-Change-Number: 20372 Gerrit-PatchSet: 11 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: Michael Smith <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Fri, 22 Sep 2023 22:00:16 +0000 Gerrit-HasComments: Yes
