Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19959 )
Change subject: IMPALA-12150: Use protocol version to isolate cluster components ...................................................................... Patch Set 13: (12 comments) More nits based on change since PS7 http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/common/global-flags.cc@384 PS13, Line 384: enter Nit: "enters" http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/common/global-flags.cc@385 PS13, Line 385: register Nit: "registers" http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/runtime/exec-env.cc@671 PS13, Line 671: // TODO wzhou Maybe remove TODO? http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/runtime/exec-env.cc@678 PS13, Line 678: as Nit: from http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/statestore/statestore-subscriber-catalog.h File be/src/statestore/statestore-subscriber-catalog.h: http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/statestore/statestore-subscriber-catalog.h@25 PS13, Line 25: It adds Catalog specifying parameters Nit: maybe just "Catalog-specific parameters" http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/statestore/statestore-subscriber.cc@362 PS13, Line 362: DebugAction(FLAGS_debug_actions, "GET_PROTOCOL_VERSION_FIRST_ATTEMPT") : Is it useful to test this in test_services_rpc_errors.py as we do for REGISTER_SUBSCRIBER_FIRST_ATTEMPT? http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/statestore/statestore-subscriber.cc@371 PS13, Line 371: imcompatitble Nit: "incompatible" http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/statestore/statestore-subscriber.cc@373 PS13, Line 373: subscriber_->GetProtocolVersion() + 1, I don't understand why we add 1 to both versions http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/19959/13/be/src/statestore/statestore.h@486 PS13, Line 486: interesting to Nit: "wants to receive" http://gerrit.cloudera.org:8080/#/c/19959/13/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: http://gerrit.cloudera.org:8080/#/c/19959/13/common/thrift/StatestoreService.thrift@223 PS13, Line 223: subscribe to the notification of updating catalogd Nit: "wishes to be notified of changes to the catalog" http://gerrit.cloudera.org:8080/#/c/19959/13/common/thrift/StatestoreService.thrift@260 PS13, Line 260: Whether Nit: "whether" http://gerrit.cloudera.org:8080/#/c/19959/13/tests/custom_cluster/test_custom_statestore.py File tests/custom_cluster/test_custom_statestore.py: http://gerrit.cloudera.org:8080/#/c/19959/13/tests/custom_cluster/test_custom_statestore.py@78 PS13, Line 78: throwed Nit: "thrown" -- To view, visit http://gerrit.cloudera.org:8080/19959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If61506dab38c4d1c50419c1b3f7bc4f9ee3676bc Gerrit-Change-Number: 19959 Gerrit-PatchSet: 13 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: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 15 Jun 2023 00:51:16 +0000 Gerrit-HasComments: Yes
