Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17266 )
Change subject: [mini-cluster] Refactor to expose flags of ExternalMaster ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/17266/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17266/1//COMMIT_MSG@10 PS1, Line 10: are > nit: were Done http://gerrit.cloudera.org:8080/#/c/17266/1/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/17266/1/src/kudu/mini-cluster/external_mini_cluster.h@729 PS1, Line 729: GetFlags > Is this method shading ExternalDaemon::GetFlags()? If so, I guess it might Done http://gerrit.cloudera.org:8080/#/c/17266/1/src/kudu/mini-cluster/external_mini_cluster.h@736 PS1, Line 736: / Get flags that are exclusive to ExternalMaster on starting it for the first time. > nit: it isn't quite clear to me based on this description how this relates Done http://gerrit.cloudera.org:8080/#/c/17266/1/src/kudu/mini-cluster/external_mini_cluster.h@737 PS1, Line 737: rpc_bind_addr > nit: document the argument? Done http://gerrit.cloudera.org:8080/#/c/17266/1/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/17266/1/src/kudu/mini-cluster/external_mini_cluster.cc@1012 PS1, Line 1012: info_path > Is it possible to derive 'info_path' from path to the first data directory, Done http://gerrit.cloudera.org:8080/#/c/17266/1/src/kudu/mini-cluster/external_mini_cluster.cc@1493 PS1, Line 1493: static vector<string> kFlags; : if (!UseLargeKeys()) { > nit: maybe, use ternary conditional to define kFlags instead of this declar With static initialization using ternary operator, kFlags will be initialized just once on the first call. So we are basically relying on UseLargeKeys() never changing it's output at runtime. That's okay with current implementation but what if implementation of UseLargeKeys() changes underneath then the flags won't get updated. That being said I'm okay with changing it as you suggested but wanted point out the reason for doing it so. Unrelated to the comment, earlier implementation had a bug that kept appending the same flags on multiple invocation. -- To view, visit http://gerrit.cloudera.org:8080/17266 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibeff3b0d6bc0021ce2aa50e8022542fb32250e07 Gerrit-Change-Number: 17266 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 07 Apr 2021 21:21:46 +0000 Gerrit-HasComments: Yes
