Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd ...................................................................... Patch Set 3: (11 comments) still reviewing, but wanted to send out some comments from my end. main question is about the complexity to use/tune the feature. http://gerrit.cloudera.org:8080/#/c/11224/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11224/3//COMMIT_MSG@12 PS3, Line 12: unused_table_ttl_sec use a common prefix for these flags so that the user knows they're related. perhaps "invalidate_tables_timeout_s"? http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc@211 PS3, Line 211: "this to 0 disables automatic invalidation of tables."); based on ttl or in general? the way I read this is that ttl and memory pressure can both be "on". the memory pressure one, if its on, can be further refined by the catalogd_table_invalidator* flags. http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc@214 PS3, Line 214: "invalidate recently unused tables when the old GC generation is almost full."); put a reference here to the two catalogd_table flags to further control behavior? http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc@216 PS3, Line 216: DEFINE_double(catalogd_table_invalidator_old_gen_full_threshold, 0.6, "The ratio above " This is a lot of new flags. Perhaps we just expose the memory_pressure one and the ttl one and make the memory related ones hidden? I wouldn't know off-hand how to tune this and would most likely just turn off the memory-based one. http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h File be/src/exec/catalog-op-executor.h: http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h@72 PS3, Line 72: servier nit: server http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h@72 PS3, Line 72: the : /// numbers of their usages nit: their usage frequency http://gerrit.cloudera.org:8080/#/c/11224/3/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/11224/3/common/thrift/CatalogService.thrift@419 PS3, Line 419: TTableUsage any reason to not simplify this to just an i32? http://gerrit.cloudera.org:8080/#/c/11224/3/common/thrift/CatalogService.thrift@460 PS3, Line 460: the numbers of their usages nit: their usage frequency http://gerrit.cloudera.org:8080/#/c/11224/3/common/thrift/CatalogService.thrift@461 PS3, Line 461: Report nit: Report is unclear who is sending whom information (is the caller getting a report or updating something?). Perhaps "UpdateTableUsage"? http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java File fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java: http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@74 PS3, Line 74: maybeAddTables nit: maybeAddTables is a bit unclear. perhaps, recordTableUsage? http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@81 PS3, Line 81: table.refreshLastUsedTime(); I forget what validate does, but if it can throw an exception, perhaps update the use after the validate? -- To view, visit http://gerrit.cloudera.org:8080/11224 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89 Gerrit-Change-Number: 11224 Gerrit-PatchSet: 3 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Mon, 27 Aug 2018 19:23:43 +0000 Gerrit-HasComments: Yes
