Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20368 )
Change subject: IMPALA-12024: Add catalog profile for createTable ...................................................................... Patch Set 3: (12 comments) > Patch Set 3: > > (12 comments) > > I think it is important to make the event label self explanatory and as clear > as possible. Otherwise, an explanation should be documented somewhere for > each label. > > Some general comments: > - Replace HMS with Metastore for consistency. > - Consider turning some duplicated event label into string constant. > - Clarify the source of event. Is it Metastore vs CatalogD vs Filesystem vs > other service (like Kudu master). Thanks for the suggestions! Refactored the patch to use string constants. I was intended to write the labels differently by strings to get rid of multiple hits when searching the code. But it makes sense to normalize them as constants so we don't need to document them. Also clarified the sources. BTW, use "Kudu" instead of "Kudu master" since we treate Kudu as a system and can't distinguish what are created in Kudu master and what in Kudu tablet servers. I plan to add more tests but need to figure out a clean way of verifying the EventSequence. Found an existing bug of the JSON profile by the way: IMPALA-12391 http://gerrit.cloudera.org:8080/#/c/20368/3/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/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3523 PS3, Line 3523: catalogTimeline.markEvent("Created Metastore table"); : } else { : catalogTimeline.markEvent("Found table exists"); > Maybe reorganize this to 2 things: existence check and new table creation: Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3552 PS3, Line 3552: Created catalog table > nit: "Cached table in catalogd"? Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3610 PS3, Line 3610: Created Metastore table > nit: "Created table in Metastore"? Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3627 PS3, Line 3627: HMS > nit: "Fetched table from Metastore"? Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3656 PS3, Line 3656: Created catalog table > nit: "Cached table in catalog"? Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3702 PS3, Line 3702: HMS > nit: Metastore Yeah, this one is different than other fetches since it just use the createTime and not caching other things. http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3786 PS3, Line 3786: Created iceberg table > nit: created in where? HMS, filesystem, or CatalogD cache? It depends on what iceberg catalog is used. Hard to say it's just HMS or filesystem.. This label generally means the invocation on iceberg createTable API finished. http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3827 PS3, Line 3827: Created Metastore table > nit: dulicated String. Change to constant? Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3834 PS3, Line 3834: Found table exists > nit: dulicated String. Change to constant? Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@102 PS3, Line 102: Checked table exists > nit: Checked table existence Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@342 PS3, Line 342: Checked table exists > nit: Checked table existence Done http://gerrit.cloudera.org:8080/#/c/20368/3/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@369 PS3, Line 369: cols > nit: columns Done -- To view, visit http://gerrit.cloudera.org:8080/20368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ebf591625e71391a5b23f56ddca8f0ae97b1efa Gerrit-Change-Number: 20368 Gerrit-PatchSet: 3 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Tue, 22 Aug 2023 02:54:37 +0000 Gerrit-HasComments: Yes
