Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19484 )
Change subject: IMPALA-11822: Optimize the Refresh/Invalidate event processing by skipping unnecessary events ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/19484/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19484/1//COMMIT_MSG@17 PS1, Line 17: Testing: Couldn't test this feature in an end-to-end I think we need our infra scripts to support launching multiple Impala clusters in our dev box. Filed IMPALA-11933. http://gerrit.cloudera.org:8080/#/c/19484/1/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/19484/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@294 PS1, Line 294: private Map<String, Map<String,Integer>> refreshMetadataMap = new HashMap<>(); Could you add a comment about the key and value types? http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2442 PS1, Line 2442: private int eventTime_; nit: need a blank line after this http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2514 PS1, Line 2514: refreshMetadataMap.getOrDefault(dbName_ + "." + tblName_ nit: we can extract the code of getting the map value http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2516 PS1, Line 2516: >= eventTime_ Is the eventTime generated by HMS? If there are time skew between the HMS node and catalogd node, we might ignore some reload events unexpectedly. Is there an HMS API that can detect the HMS local time? Or can we get the time difference (if there is) in self event detection? http://gerrit.cloudera.org:8080/#/c/19484/1/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/19484/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6362 PS1, Line 6362: } Should we update 'refreshMetadataMap' for normal refresh cases? -- To view, visit http://gerrit.cloudera.org:8080/19484 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I905957683a96c3ea01ab4bf043d6658ce37b7574 Gerrit-Change-Number: 19484 Gerrit-PatchSet: 1 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Mon, 20 Feb 2023 12:31:57 +0000 Gerrit-HasComments: Yes
