Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10171 )
Change subject: IMPALA-6916: Implement COMMENT ON DATABASE ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/10171/5/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/10171/5/common/thrift/JniCatalog.thrift@634 PS5, Line 634: enum TCommentScope { Is this enum needed? Can't we determine which comment we should update based on which fields are set in TCommentOnParams? We'll eventually need db, table, column, function. Seems like the enum is redundant. http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java File fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java: http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java@30 PS5, Line 30: Preconditions.checkNotNull(comment); In Postgres you can remove a comment with: COMMENT ON DATABASE <db> IS NULL; We should allow that, too. http://gerrit.cloudera.org:8080/#/c/10171/5/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/10171/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3524 PS5, Line 3524: Preconditions.checkNotNull(db); I don't think this is a precondition. The database could have legitimately been deleted while this COMMENT ON operation is ongoing. http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3528 PS5, Line 3528: addDbToCatalogUpdate(db, response.result); You need to increment and set the version of the db, not only the thrift representation. Have you tested whether this db modification gets propagated to all impalads and not just the coordinator? Changes like this are really tricky to get right. Let me think some more about the concurrency implications of this. Will get back with more details. http://gerrit.cloudera.org:8080/#/c/10171/5/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: http://gerrit.cloudera.org:8080/#/c/10171/5/tests/metadata/test_ddl.py@197 PS5, Line 197: def test_alter_database_comment(self, vector, unique_database): test_comment_on_database -- To view, visit http://gerrit.cloudera.org:8080/10171 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcf909c18f97073346f6f603538bf921e69fbb00 Gerrit-Change-Number: 10171 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Comment-Date: Fri, 27 Apr 2018 17:16:37 +0000 Gerrit-HasComments: Yes
