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 14: (15 comments) A few small things to look at but overall looks great. Also, please do not forget to update the ports list doc -- https://impala.apache.org/docs/build/asf-site-html/topics/impala_ports.html 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: DEFINE_string(peer_state_store_host, "localhost", Can this flag be moved to be/src/statestore/statestore.cc? Since it is statestore specific, it would make more sense there. That being said, Impala has some inconsistencies with flag placement/naming thus this is not a must do for me. Also, renaming it to 'state_store_peer_host' would better match existing naming conventions http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@437 PS14, Line 437: DEFINE_int32(peer_state_store_ha_port, 24021, Can this flag be moved to be/src/statestore/statestore.cc? Since it is statestore specific, it would make more sense there. That being said, Impala has some inconsistencies with flag placement/naming thus this is not a must do for me. Also, renaming it to 'state_store_peer_ha_port' would better match existing naming conventions http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@439 PS14, Line 439: DEFINE_bool(enable_statestored_ha, false, "Set to true to enable Statestore HA"); To better match conventions, rename this to 'enable_state_store_ha' http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@440 PS14, Line 440: DEFINE_bool(force_statestored_active, false, "Set to true to force this statestored " Can this flag be moved to be/src/statestore/statestore.cc? Since it is statestore specific, it would make more sense there. That being said, Impala has some inconsistencies with flag placement/naming thus this is not a must do for me. Also, renaming it to 'state_store_force_active' would better match existing naming conventions http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@446 PS14, Line 446: DEFINE_bool(use_network_address_as_statestored_priority, false, "Network address is " Rename to 'use_network_address_as_state_store_priority' to better match conventions. Also, is this flag applicable only to statestored? If so, can it be moved to Can this flag be moved to be/src/statestore/statestore.cc? http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/common/global-flags.cc@452 PS14, Line 452: DEFINE_int64(statestored_ha_preemption_wait_period_ms, 10000, "(Advanced) The time after " Can this flag be moved to be/src/statestore/statestore.cc? Since it is statestore specific, it would make more sense there. That being said, Impala has some inconsistencies with flag placement/naming thus this is not a must do for me. Also, renaming it to 'state_store_ha_preemption_wait_period_ms' would better match existing naming conventions 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(INFO) << Substitute("Receive UpdateState RPC request from incompatible " To me, this should be a warning instead of info. If this situation does happen, it probably means something is wrong in the cluster setup. An info level message will get lost too easily. http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@158 PS14, Line 158: LOG(INFO) << Substitute("Receive Heartbeat RPC request from incompatible " To me, this should be a warning instead of info. If this situation does happen, it probably means something is wrong in the cluster setup. An info level message will get lost too easily. http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@177 PS14, Line 177: LOG(INFO) << Substitute("Receive UpdateCatalogd RPC request from incompatible " To me, this should be a warning instead of info. If this situation does happen, it probably means something is wrong in the cluster setup. An info level message will get lost too easily. http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@205 PS14, Line 205: LOG(INFO) << Substitute("Receive UpdateStatestoredRole RPC request from " To me, this should be a warning instead of info. If this situation does happen, it probably means something is wrong in the cluster setup. An info level message will get lost too easily. http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore-subscriber.cc@461 PS14, Line 461: // DCHECK(active_statestore_ != nullptr); Nit: please either remove or uncomment this code. 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 conventions for flag names. http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore.cc@1785 PS14, Line 1785: satestored nit: misspelled 'satestored' in thread name. http://gerrit.cloudera.org:8080/#/c/20372/14/be/src/statestore/statestore.cc@1983 PS14, Line 1983: // TODO (wzhou): Need better approach to handle split-brain in network. Please reference an IMPALA Jira here. 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 they are not necessary due to order of operations rules: DEFAULT_STATESTORE_HA_SERVICE_PORT + ((instance_num + 1) % 2) -- 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: 14 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: Wed, 18 Oct 2023 18:56:16 +0000 Gerrit-HasComments: Yes
