Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19530 )
Change subject: IMPALA-11940: [DOCS] Document manifest caching settings for Iceberg ...................................................................... Patch Set 2: (7 comments) I did a quick read through http://gerrit.cloudera.org:8080/#/c/19530/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19530/2//COMMIT_MSG@9 PS2, Line 9: IMPALA-11658 implement Iceberg manifest caching config for Impala. This Nit: "implements Iceberg manifest caching for Impala." http://gerrit.cloudera.org:8080/#/c/19530/2//COMMIT_MSG@10 PS2, Line 10: patch add documentation for that settings. Nit: "adds documentation for configuring the cache(s). http://gerrit.cloudera.org:8080/#/c/19530/2/docs/topics/impala_iceberg.xml File docs/topics/impala_iceberg.xml: http://gerrit.cloudera.org:8080/#/c/19530/2/docs/topics/impala_iceberg.xml@617 PS2, Line 617: by setting properties in Hadoop's core-site.xml as the following example: Nit: "as in the following" http://gerrit.cloudera.org:8080/#/c/19530/2/docs/topics/impala_iceberg.xml@631 PS2, Line 631: custom FileIO implementation to use in a catalog. Must be set to enable Do we recommend that this not be changed? Maybe we should say that? http://gerrit.cloudera.org:8080/#/c/19530/2/docs/topics/impala_iceberg.xml@632 PS2, Line 632: manifest caching. Impala default to HadoopFileIO. Nit: "defaults to" http://gerrit.cloudera.org:8080/#/c/19530/2/docs/topics/impala_iceberg.xml@640 PS2, Line 640: of a manifest file to be considered for caching. Manifest files with a length Why would I want to set this to anything other than a high value? Oh. I see, is it because of max-total-bytes? So we should set this to a value less than max-total-bytes? http://gerrit.cloudera.org:8080/#/c/19530/2/docs/topics/impala_iceberg.xml@652 PS2, Line 652: <codeph>iceberg.io.manifest.cache.max-total-bytes</codeph>: maximum total Maybe move this before iceberg.io.manifest.cache.expiration-interval-ms That way the explanation in interval-ms that refers to max-total-bytes will be easier to read. If we do this, maybe change the example code block too. -- To view, visit http://gerrit.cloudera.org:8080/19530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd761a81f5c81a25a5ec0889402f85157c23e9fe Gerrit-Change-Number: 19530 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 24 Feb 2023 02:03:14 +0000 Gerrit-HasComments: Yes
