Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 )
Change subject: KUDU-2543 pt 1: basic checks for authz tokens ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_verification_util.cc File src/kudu/rpc/rpc_verification_util.cc: http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_verification_util.cc@59 PS7, Line 59: return Status::NotAuthorized(VerificationResultToString(result)); > Done Hmm, wouldn't a LOG(FATAL) in the default case be safer? If we miss the compilation warning, and someone runs this code, the function will end up returning an error but with 'error' unset. http://gerrit.cloudera.org:8080/#/c/11751/8/src/kudu/tserver/tablet_server_authorization-test.cc File src/kudu/tserver/tablet_server_authorization-test.cc: http://gerrit.cloudera.org:8080/#/c/11751/8/src/kudu/tserver/tablet_server_authorization-test.cc@104 PS8, Line 104: struct RequestGeneratorParam { Are the struct and the lambdas necessary? Could you just define each generator as a static function, then wrap them up in a vector<std::function<...>>? Or better yet, avoid the vector altogether and use an initializer list in testing::ValuesIn? I mean it works this way, but it's unusual to see lambdas defined in that scope, and it's not clear why the wrapping struct is necessary. http://gerrit.cloudera.org:8080/#/c/11751/8/src/kudu/tserver/tablet_server_authorization-test.cc@175 PS8, Line 175: CHECK_OK(SchemaToColumnPBs(schema, scan->mutable_projected_columns())); Could RETURN_NOT_OK here? And in the other generators? http://gerrit.cloudera.org:8080/#/c/11751/8/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11751/8/src/kudu/tserver/tablet_service.cc@451 PS8, Line 451: DCHECK(req->GetTypeName() == "NewScanRequestPB" || : req->GetTypeName() == "SplitKeyRangeResponse") << req->GetTypeName(); Why is this necessary? Won't compilation fail if an AuthorizableScanRequest without authz_token() is used? -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 03 Jan 2019 04:50:57 +0000 Gerrit-HasComments: Yes
