Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement ...................................................................... Patch Set 12: (14 comments) Coming along nicely. A few more 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: public enum Action { Nit: less cluttered to put enums, nested classes at the top of the class, followed by statics, then non-static members http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@122 PS12, Line 122: Action getAction() { return action_; } Nit: include "protected" keyword. It is the default, yes, but convenient for readers to add it. 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 http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@182 PS12, Line 182: Preconditions.checkState(false, "Invalid reset metadata action: " + action_); Nit: This is kind of overkill. Simply throw IllegalStateException("Invalid... is more direct. If you look in Preconditions.checkState, that's all it will do. http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@190 PS12, Line 190: if (action_.isRefresh_) { action_.isRefresh() 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"); Actually, this whole block can be simpler: if (action_.isRefresh()) result.append("REFRESH "); Then, the two switch statements can be combined into one. Or, like was done with INVALIDATE, just include the REFRESH keyword in the individual case. http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@204 PS12, Line 204: .append(" " + partitionSpec_.toSql(options)); Nit: .append(" ").append(partitionSpec... http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@207 PS12, Line 207: Preconditions.checkState(false, "Invalid reset metadata action: " + action_); Nit: see above http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@216 PS12, Line 216: result.append("INVALIDATE METADATA").append(" ").append(tableName_); Nit: result.append("INVALIDATE METADATA").append(" ") --> result.append("INVALIDATE METADATA ") Also, ToSqlUtils.getIdentSql(tableName_) . Handles the case where table name is a keyword, say, "select". http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@219 PS12, Line 219: Preconditions.checkState(false, "Invalid reset metadata action: " + action_); Nit: see above http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@228 PS12, Line 228: params.setIs_refresh(action_.isRefresh_); Use method 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 removed? Better name? Also, anything we need to do with the added list? If not, maybe include a comment to explain why? 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: List<TCatalogObject> added, List<TCatalogObject> removed) The arguments are getting complex. And, it is awkward to return values via arguments. Consider defining a RefreshAction class of some sort that takes the various options, is passed in here, and can return the results. Looks like we already have something similar above with the PolicyReader class. Can we either use that, or create another class like that mentioned above? http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/util/SentryProxy.java@305 PS12, Line 305: List<TCatalogObject> added, List<TCatalogObject> removed) This one is also getting cluttered. -- 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: 12 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 01:03:06 +0000 Gerrit-HasComments: Yes
