Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-5355: Fix the order of Sentry roles and privileges ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/7004/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: PS1, Line 1203: Preconditions.checkState(role.checkVersionConsistency()); I don't think this check is needed here. I don't see how, given that the catalogLock is held, you could end up in an inconsistent state. PS1, Line 1224: Preconditions.checkState(role.checkVersionConsistency()); same here http://gerrit.cloudera.org:8080/#/c/7004/1/fe/src/main/java/org/apache/impala/catalog/Role.java File fe/src/main/java/org/apache/impala/catalog/Role.java: Line 122: Preconditions.checkState(result.checkVersionConsistency()); add an error message here, something to indicate the problem. http://gerrit.cloudera.org:8080/#/c/7004/1/tests/authorization/test_impalad_restart.py File tests/authorization/test_impalad_restart.py: PS1, Line 30: TestImpaladRestart Add description of what kind of tests we plan on adding in this class. PS1, Line 50: test_impalad_restart This function name is a bit generic and it's not clear what this test case is all about. PS1, Line 51: correclty typo: correctly PS1, Line 57: assert It's also important for this test that there are privileges associated with this role. You may want to add a show grant privileges request to verify that this is indeed the case. The same check should be added after the restart. PS1, Line 64: sleep(10) I believe there is a better way to detect this. I think there is a metric (CATALOG_READY or something like this) you can query that indicates if the impalad has received the catalog update and is ready to process requests. -- To view, visit http://gerrit.cloudera.org:8080/7004 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7072e95b74952ce5a51ea1b6e2ae3e80fb0940e0 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
