Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/21546 )
Change subject: IMPALA-9441,IMPALA-13170: Ops listing dbs/tables should handle db not exists ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/21546/2/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/21546/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1171 PS2, Line 1171: call() My understanding is that this function checks the authorization for either a database or a table. To improve readability, can we refactor the call() method to call two separate functions? Something like: @Override public Boolean call() throws Exception { if (isDbCheck()) { return checkDb(); } else { Preconditions.checkState(isTableCheck(), "..."); return checkTable(); } } private boolean checkDb() throws Exception { try { // ... return isAccessibleToUser(db_.getName(), null, db_.getOwnerUser(), user_); } catch (InconsistentMetadataFetchException e) { Preconditions.checkState(e.getReason() == CatalogLookupStatus.DB_NOT_FOUND, "Unexpected failure of InconsistentMetadataFetchException: %s", e.getReason()); LOG.warn("Database {} no longer exists", db_.getName(), e); } return false; } private boolean checkTable() throws Exception { // ... String tableOwner = table_.getOwnerUser(); if (tableOwner == null) { LOG.info("Table {} not yet loaded, ignoring it in table listing.", table_.getFullName()); } return isAccessibleToUser(table_.getDb().getName(), table_.getName(), tableOwner, user_); } private boolean isDbCheck() { return db_ != null; } private boolean isTableCheck() { return table_ != null; } http://gerrit.cloudera.org:8080/#/c/21546/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1171 PS2, Line 1171: call Another option is to use different classes for each, which seems more in line with OOP. private abstract class CheckAuthorization implements Callable<Boolean> { protected final User user_; public CheckAuthorization(User user) { Preconditions.checkNotNull(user); this.user_ = user; } public abstract boolean checkAuthorization() throws Exception; @Override public Boolean call() throws Exception { return checkAuthorization(); } } private class CheckDbAuthorization extends CheckAuthorization { private final FeDb db_; public CheckDbAuthorization(FeDb db, User user) { super(user); Preconditions.checkNotNull(db); this.db_ = db; } @Override public boolean checkAuthorization() throws Exception { try { return isAccessibleToUser(db_.getName(), null, db_.getOwnerUser(), user_); } catch (InconsistentMetadataFetchException e) { Preconditions.checkState(e.getReason() == CatalogLookupStatus.DB_NOT_FOUND, "Unexpected failure of InconsistentMetadataFetchException: %s", e.getReason()); LOG.warn("Database {} no longer exists", db_.getName(), e); } return false; } } private class CheckTableAuthorization extends CheckAuthorization { private final FeTable table_; public CheckTableAuthorization(FeTable table, User user) { super(user); Preconditions.checkNotNull(table); this.table_ = table; } @Override public boolean checkAuthorization() throws Exception { String tableOwner = table_.getOwnerUser(); if (tableOwner == null) { LOG.info("Table {} not yet loaded, ignoring it in table listing.", table_.getFullName()); } return isAccessibleToUser(table_.getDb().getName(), table_.getName(), tableOwner, user_); } } -- To view, visit http://gerrit.cloudera.org:8080/21546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd40d33859feca2bbd2e5f1158f3894a91c2929 Gerrit-Change-Number: 21546 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Fri, 28 Jun 2024 18:53:22 +0000 Gerrit-HasComments: Yes
