Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement ...................................................................... Patch Set 11: (7 comments) The code looks fine to me. I still have some concerns around correctness. http://gerrit.cloudera.org:8080/#/c/11888/11/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/11888/11/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@138 PS11, Line 138: String dbName = analyzer.getTargetDbName(tableName_); Preconditions.checkNotNull(tableName_) between L137-138 http://gerrit.cloudera.org:8080/#/c/11888/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11888/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1140 PS8, Line 1140: blic void refreshAuthorizat > Actually we set sentryProxy_ to null in https://github.com/apache/impala/bl Can we reverse it then. if (sentryProxy == null) return; http://gerrit.cloudera.org:8080/#/c/11888/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11888/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3451 PS8, Line 3451: catalog_.refreshAuthorization(false); > We actually increment the catalog version in L3452. I did a quick experimen Actually thinking a bit more about this, I still am not convinced. I think there are a few subtle problems here. 1. catalog_.getCatalogVersion() could potentially return a value > what we need here. Reason being that since you are not locking the versions globally, there could potentially be concurrent operations that increment versions between L3451 and L3452. 2. I still am not convinced by the correctness of this. If you see the coordinator side synchronous wait loop for this, it looks like follows, Status ImpalaServer::ProcessCatalogUpdateResult( const TCatalogUpdateResult& catalog_update_result, bool wait_for_all_subscribers) { const TUniqueId& catalog_service_id = catalog_update_result.catalog_service_id; if (!catalog_update_result.__isset.updated_catalog_objects && !catalog_update_result.__isset.removed_catalog_objects) { // Operation with no result set. Use the version specified in // 'catalog_update_result' to determine when the effects of this operation // have been applied to the local catalog cache. if (catalog_update_result.is_invalidate) { WaitForMinCatalogUpdate(catalog_update_result.version, catalog_service_id); } else { WaitForCatalogUpdate(catalog_update_result.version, catalog_service_id); } In this case since you are setting the version, you'd block on WaitForCatalogUpdate(catalog_update_result.version, catalog_service_id). However, since SS does not guarantee broadcast in the order of versions, there is a chance that the loop in WaitForCatalogUpdate() might break early. If you see invalidate metadata (which uses a similar logic), you see it calls WaitForMinCatalogUpdate(). Notice the *Min* part. 3. I'm afraid that if we take the invalidate approach, this becomes incompatible with Catalog V2 (which does not include an easy way of finding min versions on the coordinator). Please let me know if I'm wrong in my understanding. http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_authorization.py@546 PS8, Line 546: Can we also do execute_query_expect_failure(select from functional.alltypes) here. http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_authorization.py@555 PS8, Line 555: finally: execute_query_expect_success() http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_grant_revoke.py@211 PS8, Line 211: """Tests that explicit grant/revoke within Impala and external privilege update should : produce correct privileges in Impala after a Sentry refresh.""" : group_name = grp.getgrnam(getuser()).gr_name : try: : self.client.execute("create role %s" % unique_role) : self.client.execute("grant role %s to group `%s`" % (unique_role, group_name)) : # Add select and insert into the catalog. : self.client.execute("grant refresh on server to %s" % unique_role) : self.client.execute("grant select on table functional.alltypes to %s" % unique_role) : self.client.execute("grant insert on table functional.alltypes to %s" % unique_role) : # Grant alter privilege and revoke insert privilege using Sentry CLI. : subprocess.check_call( : ["/bin/bash", "-c", : "%s/bin/sentryShell --conf %s/sentry-site.xml -gpr -p " : "'server=server1->db=functional->table=alltypes->action=alter' -r %s" % : (os.getenv("SENTRY_HOME"), os.getenv("SENTRY_CONF_DIR"), unique_role)], : stdout=sys.stdout, stderr=sys.stderr) : subprocess.check_call( : ["/bin/bash", "-c", : "%s/bin/sentryShell --conf %s/sentry-site.xml -rpr -p " : "'server=server1->db=functional->table=alltypes->action=insert' -r %s" % : (os.getenv("SENTRY_HOME"), os.getenv("SENTRY_CONF_DIR"), unique_role)], : stdout=sys.stdout, stderr=sys.stderr) : : self.client.execute("refresh authorization") : : # Ensure alter privilege was granted and insert privilege was revoked. : result = self.execute_query_expect_success(self.client, "show grant role %s" % : unique_role) : assert len(result.data) == 3 : assert any("select" in x for x in result.data) : assert any("alter" in x for x in result.data) : assert any("refresh" in x for x in result.data) : finally: : self.client.execute("drop role %s" % unique_role) can we fold this into test_sentry_refresh? http://gerrit.cloudera.org:8080/#/c/11888/8/tests/common/sentry_cache_test_suite.py File tests/common/sentry_cache_test_suite.py: http://gerrit.cloudera.org:8080/#/c/11888/8/tests/common/sentry_cache_test_suite.py@67 PS8, Line 67: refresh_authorization), indentation looks weird but I'm assuming it is PEP compatible since the jenkins job is not complaining. -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Fri, 07 Dec 2018 06:45:48 +0000 Gerrit-HasComments: Yes
