Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/20372 )
Change subject: IMPALA-12156: Support High Availability for Statestore ...................................................................... Patch Set 15: (20 comments) A few more thoughts/questions. 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 > moved to statestore.cc and renamed to state_store_peer_host Done 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"; > moved to statestore.cc and renamed as 'state_store_peer_ha_port' Done 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' Done http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@440 PS14, Line 440: "Specifies which implementation of " > moved to statestore.cc and renamed as statestored_force_active Done 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 Done http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@452 PS14, Line 452: // / R I P ╲ -----------|----------- > moved to statestore.cc Done http://gerrit.cloudera.org:8080/#/c/20372/15/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/20372/15/be/src/common/global-flags.cc@426 PS15, Line 426: // Starting flags for Statestore HA Thoughts on moving these flags to the same location where their corresponding original flags are? For example, 'state_store_host' is defined in exec-env.cc while 'state_store_port' is defined in statestore.cc. Does that mean 'state_store_2_host' should also go into exec-env.cc and 'state_store_2_port' go into statestore.cc? I don't like have the flags spread out across so many files, but I also prefer to stick with existing code layout. http://gerrit.cloudera.org:8080/#/c/20372/15/be/src/common/global-flags.cc@432 PS15, Line 432: DEFINE_int32(state_store_ha_port, 24020, Given the existing flags are named '-state_store_host' and '-state_store_port', I think we should stick with these flags also having a name that starts with '-state_store_' instead of '-statestore_' 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 " > changed to warning Done 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 " > changed to warning Done 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 " > changed to warning Done 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 " > WARNING Done http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@461 PS14, Line 461: StatestoreSubscriber::StatestoreStub* StatestoreSubscriber::GetActiveStatestore() { > removed Done 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 existing flags in this source file are started with 'statestore'. Done http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore.cc@115 PS14, Line 115: // Flags for Statestore HA > It looks like we're somewhat inconsistent. Some use state_store, others use Good point, I did some more investigating, and flag names definitely favor starting with '-statestore_' instead of '-state_store_'. I did not find any other instance of a flag starting with '-statestored', thus I agree we should drop the 'd'. Overall, I agree that the new flags should start with '-statestore_' instead of my original thought of starting with '-state_store_' 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 Done http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore.cc@1785 PS14, Line 1785: > fixed Done 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); > Filed a jira IMPALA-12507 Done http://gerrit.cloudera.org:8080/#/c/20372/15/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/20372/15/be/src/statestore/statestore.cc@122 PS15, Line 122: DEFINE_bool(statestored_force_active, false, "Set to true to force this statestored " Please rename to 'statestore_force_active' http://gerrit.cloudera.org:8080/#/c/20372/15/be/src/statestore/statestore.cc@134 PS15, Line 134: DEFINE_int64(statestored_ha_preemption_wait_period_ms, 10000, "(Advanced) The time after " Please rename to 'statestore_ha_preemption_wait_period_ms' -- 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 20:12:53 +0000 Gerrit-HasComments: Yes
