Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement ...................................................................... Patch Set 13: (14 comments) http://gerrit.cloudera.org:8080/#/c/11888/12/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/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@56 PS12, Line 56: > Nit: less cluttered to put enums, nested classes at the top of the class, f Done http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@122 PS12, Line 122: protected Action getAction() { return action_; } > Nit: include "protected" keyword. It is the default, yes, but convenient fo Done http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@141 PS12, Line 141: if (action_.isRefresh()) { > action_.isRefresh() -- use method, not private field Done http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@182 PS12, Line 182: throw new IllegalStateException("Invalid reset metadata action: " + action_); > Nit: This is kind of overkill. Simply throw IllegalStateException("Invalid. Done http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@190 PS12, Line 190: case REFRESH_AUTHORIZATION: > action_.isRefresh() Done http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@191 PS12, Line 191: result.append("REFRESH AUTHORIZATION"); > Actually, this whole block can be simpler: Originally I was following the old style, but I agree I don't think there's any point saving few words with more complicated if/else/switch. Done. http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@204 PS12, Line 204: result.append("INVALIDATE METADATA"); > Nit: .append(" ").append(partitionSpec... Done http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@207 PS12, Line 207: result.append("INVALIDATE METADATA ").append(tableName_.toSql()); > Nit: see above Done http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@216 PS12, Line 216: TResetMetadataRequest params = new TResetMetadataRequest(); > Nit: result.append("INVALIDATE METADATA").append(" ") --> result.append("IN Good catch! Done. http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@219 PS12, Line 219: params.setTable_name(new TTableName(tableName_.getDb(), tableName_.getTbl())); > Nit: see above Done http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@228 PS12, Line 228: } > Use method Done http://gerrit.cloudera.org:8080/#/c/11888/12/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/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3458 PS12, Line 3458: catalog_.getDeleteLog().addRemovedObject(obj); > addRemovedObject? Does this really add to the catalog an object that was re Yup, that's what addRemovedObject means. I'd rather not rename anything in the existing catalog code for now. No, deletion is handled differently in the catalog, hence the need to have getDeleteLog(). http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/util/SentryProxy.java@265 PS12, Line 265: refreshPrivilegesInCatalog(catalog, resetVersions, sentryRole.getRoleName(), role, > The arguments are getting complex. And, it is awkward to return values via It's a bit trickier since the whole thing runs in a thread so there really isn't a good way to use a return type unless we modify the whole thing to return a Future, but I think that's a better task for another CR. However, I do agree that the number of arguments are getting unwieldy. In the next PS, I introduced a private class AuthorizationCatalog to capture the added/removed catalog objects. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/util/SentryProxy.java@305 PS12, Line 305: user, allUsersPrivileges, authzCatalog); > This one is also getting cluttered. Done -- 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: 13 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Fri, 14 Dec 2018 19:34:33 +0000 Gerrit-HasComments: Yes
