Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19231 )
Change subject: KUDU-3357 endpoints for proxied RPCs ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/19231/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19231/4//COMMIT_MSG@23 PS4, Line 23: <dns_name|ip_addr>:<port> > nit: put quotes around this, the trailing colon makes it look like a part i Done http://gerrit.cloudera.org:8080/#/c/19231/4//COMMIT_MSG@23 PS4, Line 23: dns_name > nit: hostname Done http://gerrit.cloudera.org:8080/#/c/19231/4//COMMIT_MSG@27 PS4, Line 27: outer > Maybe we should use "external" and "internal" when specifying the network t Done http://gerrit.cloudera.org:8080/#/c/19231/4//COMMIT_MSG@58 PS4, Line 58: kudu perf scan > kudu table scan or kudu perf table_scan? Done http://gerrit.cloudera.org:8080/#/c/19231/4//COMMIT_MSG@58 PS4, Line 58: A test Kudu Java client : application worked as well > Does it make sense to have a test where we write and read from a table from Done. http://gerrit.cloudera.org:8080/#/c/19231/4//COMMIT_MSG@61 PS4, Line 61: NOTE: even if "kudu cluster ksck" tool also worked, it's not yet a goal > This is a bit confusing for me, does kudu cluster ksck work? If not, is the Yes, it works since MasterServiceImpl::ListMasters() and MasterServiceImpl::ListTabletServers() were updated as well. However, at this point it's not a goal to update all the kudu CLI tools used to administer a Kudu cluster, and that's what this comment is about. http://gerrit.cloudera.org:8080/#/c/19231/4/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: http://gerrit.cloudera.org:8080/#/c/19231/4/src/kudu/common/wire_protocol.proto@94 PS4, Line 94: proxy_rpc_addresses > nit: Do you think having this named to rpc_proxy_addresses is more consiste Done http://gerrit.cloudera.org:8080/#/c/19231/4/src/kudu/integration-tests/create-table-itest.cc File src/kudu/integration-tests/create-table-itest.cc: http://gerrit.cloudera.org:8080/#/c/19231/4/src/kudu/integration-tests/create-table-itest.cc@585 PS4, Line 585: hp0 > nit: Maybe call this hp to keep it consistent with below? Done http://gerrit.cloudera.org:8080/#/c/19231/4/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/19231/4/src/kudu/master/ts_descriptor.cc@220 PS4, Line 220: DCHECK_GT(reg->proxy_rpc_addresses_size(), 0); > what's the purpose of this dcheck here? The idea was to check that tablet servers are run with proper settings (i.e. with proxy RPC addresses defined) when this method is called with use_external_addr=true. I updated the signature of this method to check on that in a more regular way instead. http://gerrit.cloudera.org:8080/#/c/19231/4/src/kudu/tserver/heartbeater.cc File src/kudu/tserver/heartbeater.cc: http://gerrit.cloudera.org:8080/#/c/19231/4/src/kudu/tserver/heartbeater.cc@364 PS4, Line 364: // The RPC proxy-advertised addresses and the proxied RPC addresses should > Shouldn't we compare the sizes? Or is it valid for the sizes be different? The sizes might be different, yes. I added a comment about that. -- To view, visit http://gerrit.cloudera.org:8080/19231 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic300250556d3f6e522a71923bed6aa5cd45375ea Gerrit-Change-Number: 19231 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Sat, 10 Dec 2022 17:45:26 +0000 Gerrit-HasComments: Yes
