Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13013 )
Change subject: master: use AuthzProvider to generate authz tokens ...................................................................... Patch Set 7: (10 comments) Looks great overall, just a few nits on the test. http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc File src/kudu/integration-tests/ts_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@106 PS7, Line 106: privilege policy nit: What is 'privilege policy'? I'm not sure, but I guess that's something related to the actual authorization of various actions based on granted privileges? http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@138 PS7, Line 138: inverse privileges nit: what is an 'inverse' privilege? As I understand, Sentry doesn't have 'deny' privilege, so I'm lost what it might be. From the code it seems to be some sort of complement to the kFullPrivileges. Maybe, replace 'inverse' with 'complement' then? http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@184 PS7, Line 184: break; nit here and below: is it necessary to have 'break' if there is 'return'? http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@264 PS7, Line 264: SKIP_IF_SLOW_NOT_ALLOWED(); This is fragile: it requires the same macro in the body of the test, otherwise it might crash or behave unpredictable, and the latter is hard to troubleshoot. Please at least add a comment that the same macro should be the very first thing in the body of every test scenario based on this class (I think it's better to add that note in the class-wide comment). http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@279 PS7, Line 279: kudu, nit: could you please add a comment explaining why it's necessary to have 'kudu' user among regular users for master? It looks a bit strange if looking at this without much context. http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@323 PS7, Line 323: ->NotNull() nit: is it crucial to have the non-null constraint for some particular purpose? If not, maybe drop it? http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@365 PS7, Line 365: const vector<string> table_names = std::move(tables); nit: I'm curious why it's necessary to do this. This invalidates 'tables', while it's still in the scope. Maybe, if you wanted const vector, just create a const reference to tables instead? http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@452 PS7, Line 452: ASSERT_TRUE(s.IsRemoteError()) Ah, so that's not an application-level error? I.e., the only way to distinguish it is looking at the error string, but not the status code? At some point I was under impression it will send us Status::NotAuthorized() as is, no? http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@459 PS7, Line 459: for (auto& t : threads) { : t.join(); : } It's better to add scoped cleanup, because if something above fails due to some assert in the main thread, some threads might be destroyed ungracefully and test might crash. http://gerrit.cloudera.org:8080/#/c/13013/7/src/kudu/integration-tests/ts_sentry-itest.cc@465 PS7, Line 465: TestAlters Does it make sense to try altering (at least independent) tables in parallel? -- To view, visit http://gerrit.cloudera.org:8080/13013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5404d6437699bc6c7c8bb0e530b202109e8f166 Gerrit-Change-Number: 13013 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 20 Apr 2019 06:38:14 +0000 Gerrit-HasComments: Yes
