Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16955 )
Change subject: allow customize sasl_proto_name instead of always using "kudu" ...................................................................... Patch Set 1: (6 comments) Thank you for the patch! http://gerrit.cloudera.org:8080/#/c/16955/1//COMMIT_MSG Commit Message: PS1: I think it would be great to have a test to make sure this works as expected and to catch further regressions in the code, if any. You can check various tests under src/kudu/integration-tests (e.g. security-itest.cc, etc.) for the reference/example. http://gerrit.cloudera.org:8080/#/c/16955/1//COMMIT_MSG@8 PS1, Line 8: Please add a detailed description of this changelist: motivation, etc. How can people use this new functionality and what issue(s) it solves? Is there corresponding JIRA already opened in this context? If yes, add information about that as well (you can see how it's done in other already submitted changelists in the repo). This might be helpful as well: https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines (that link can be found in https://kudu.apache.org/docs/contributing.html#_submitting_patches section). http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@58 PS1, Line 58: sasl_proto_name I'm not sure this is the best name for the flag if thinking about end-users, not just readers of the code in this file. http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@59 PS1, Line 59: Default is kudu. This is not needed: a help string for a gflag shows the default value automatically. http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@60 PS1, Line 60: sasl_proto_name Can any string be supplied here and work just fine (an empty one, with spaces, etc.)? I think it's worth adding a validator for the flag to accept only strings which can be used by the SASL library at least. http://gerrit.cloudera.org:8080/#/c/16955/1/src/kudu/rpc/messenger.cc@81 PS1, Line 81: FLAGS_sasl_proto_name Why not to use the same approach as for the rpc_authentication_ field, where the value for a particular Messenger instance is set via MessengerBuilder::set_sasl_proto_name() method in server_base.cc? For the reference, see https://github.com/apache/kudu/blob/6f5f100c8443353c007ecf375b76ba940c687f98/src/kudu/server/server_base.cc#L512-L527 -- To view, visit http://gerrit.cloudera.org:8080/16955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I79ccd0441f1d5eb087e2545e1ca8055078da5566 Gerrit-Change-Number: 16955 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Jan 2021 05:04:59 +0000 Gerrit-HasComments: Yes
