Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20372 )
Change subject: IMPALA-12156: Support High Availability for Statestore ...................................................................... Patch Set 15: (15 comments) http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@435 PS14, Line 435: // TGeospatialLibrary's values are mapped here as constants > Can this flag be moved to be/src/statestore/statestore.cc? Since it is sta moved to statestore.cc and renamed to state_store_peer_host http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@437 PS14, Line 437: static const string geo_lib_hive_esri = "HIVE_ESRI"; > Can this flag be moved to be/src/statestore/statestore.cc? Since it is sta moved to statestore.cc and renamed as 'state_store_peer_ha_port' http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@439 PS14, Line 439: static const string geo_lib_help_msg = > To better match conventions, rename this to 'enable_state_store_ha' moved to statestore.cc http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@440 PS14, Line 440: "Specifies which implementation of " > Can this flag be moved to be/src/statestore/statestore.cc? Since it is sta moved to statestore.cc and renamed as statestored_force_active http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@446 PS14, Line 446: // ++========================++ > Rename to 'use_network_address_as_state_store_priority' to better match con moved to statestore.cc http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@452 PS14, Line 452: // / R I P ╲ -----------|----------- > Can this flag be moved to be/src/statestore/statestore.cc? Since it is sta moved to statestore.cc http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@125 PS14, Line 125: LOG(WARNING) << Substitute("Receive UpdateState RPC request from incompatible " > To me, this should be a warning instead of info. If this situation does ha changed to warning http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@158 PS14, Line 158: LOG(WARNING) << Substitute("Receive Heartbeat RPC request from incompatible " > To me, this should be a warning instead of info. If this situation does ha changed to warning http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@177 PS14, Line 177: LOG(WARNING) << Substitute("Receive UpdateCatalogd RPC request from incompatible " > To me, this should be a warning instead of info. If this situation does ha changed to warning http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@205 PS14, Line 205: LOG(WARNING) << Substitute("Receive UpdateStatestoredRole RPC request from " > To me, this should be a warning instead of info. If this situation does ha WARNING http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@461 PS14, Line 461: StatestoreSubscriber::StatestoreStub* StatestoreSubscriber::GetActiveStatestore() { > Nit: please either remove or uncomment this code. removed http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore.cc@115 PS14, Line 115: // Flags for Statestore HA > All these flags should start with 'state_store' to better match existing co All existing flags in this source file are started with 'statestore'. http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore.cc@1785 PS14, Line 1785: > nit: misspelled 'satestored' in thread name. fixed http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore.cc@1983 PS14, Line 1983: ha_standby_ss_failure_detector_->GetPeerState(STATESTORE_ID); > Please reference an IMPALA Jira here. Filed a jira IMPALA-12507 http://gerrit.cloudera.org:8080/#/c/20372/14/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/20372/14/bin/start-impala-cluster.py@336 PS14, Line 336: DEFAULT_STATESTORE_HA_SERVICE_PORT + ((instance_num + 1) % 2) > Nit: additional parentheses would make this code more readable even though changed as suggested -- 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: 15 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: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 19 Oct 2023 06:26:48 +0000 Gerrit-HasComments: Yes
