Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16403 )
Change subject: KUDU-2574: Add a unique cluster ID ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/16403/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16403/4//COMMIT_MSG@14 PS4, Line 14: masters list nit: 'kudu master list' http://gerrit.cloudera.org:8080/#/c/16403/4//COMMIT_MSG@17 PS4, Line 17: enhanced and nit: drop ? http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/common/wire_protocol.proto@123 PS4, Line 123: master servers masters http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/common/wire_protocol.proto@124 PS4, Line 124: optional string cluster_id = 5; Is it possible to avoid putting it here and keep cluster_id only in GetMasterRegistrationResponsePB ? http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/catalog_manager.cc@1042 PS4, Line 1042: *cluster_id Otherwise an address would be printed, I guess. http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/master.h File src/kudu/master/master.h: http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/master.h@78 PS4, Line 78: std::string cluster_id() { return cluster_id_; } nit: add 'const' specifier -- this is a truly constant method http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/master.cc@337 PS4, Line 337: local_entry.set_cluster_id(cluster_id_); Is it possible that this method is called when cluster_id_ isn't populated yet? If so, maybe put this under 'if (!cluster_id_.empty())' clause? http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/sys_catalog-test.cc File src/kudu/master/sys_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/sys_catalog-test.cc@443 PS4, Line 443: nit: add 'const' to express it's immutable in this scenario? http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/sys_catalog-test.cc@457 PS4, Line 457: nit: could you put 'init_id' as the first argument? The reason is that it's easier to read an error messages when this assertion triggers (if ever). http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/16403/4/src/kudu/master/sys_catalog.cc@761 PS4, Line 761: p(entry); nit: it might be entries.front() :) entries.front() is a bit safer because it would generate out-of-bounds exception if the container is empty, but if you prefer entries[0] for other reason, it's fine with me. -- To view, visit http://gerrit.cloudera.org:8080/16403 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4df1d3f7d100336f52563f3008cacf6d9e328fae Gerrit-Change-Number: 16403 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 02 Sep 2020 23:37:49 +0000 Gerrit-HasComments: Yes
