Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12987 )
Change subject: IMPALA-8341: Data cache for remote reads ...................................................................... Patch Set 1: (3 comments) > Another option would be to do a global hashtable and have the > entries directly identify their partitions (in fact I think they > already do via the CacheFile pointer, right?). Eviction still needs > to be per-partition based on that partition's capacity, but > insertion can try to more smartly allocate across drives. > Yes, I had a similar question when thinking over the patch over the weekend. I tried the approach of weighted sampling + a global LRU cache but stopped short of coming up with a way to maintain the quota per partition with a global LRU list so I abandoned it and kind of punted on this problem for now. May be it's okay to do LRU per partition. Should have documented my thought there in the code. I think the _IO_ apportioned relative to capacity may be fine as that's a reasonable expectation if a user specifies more capacity on a slower device. One thing to consider may be to use multiple backing files for larger partition to avoid per-file lock problem. > Last thought: since there are some tricky design decisions above, > maybe we can simplify this and just support a single partition for > now, and come back to supporting multi-partition? It may be reasonable to punt on it for now. Let me see how much work it is to switch over to the global hash-table + per-partition LRU approach. http://gerrit.cloudera.org:8080/#/c/12987/1/be/src/runtime/io/data-cache.h File be/src/runtime/io/data-cache.h: http://gerrit.cloudera.org:8080/#/c/12987/1/be/src/runtime/io/data-cache.h@53 PS1, Line 53: /// punching holes in the backing file it's stored in. As a backing file reaches a certain > This does mean that the number of backing files could grown unbounded right It's actually a TODO item to retire older files and copy what's left in them to the current file. I didn't get around to doing that in this first version. http://gerrit.cloudera.org:8080/#/c/12987/1/be/src/runtime/io/data-cache.h@56 PS1, Line 56: /// > We should mention in the comment how lookup works, in particular what happe It's by design that we don't support overlapping range for the first implementation. It seems to fare pretty well with the TPC-DS workload and parquet file format we are using. One of the TODO in the future is to measure the overlapping case and consider handling them if they are actually common. http://gerrit.cloudera.org:8080/#/c/12987/1/be/src/runtime/io/data-cache.cc File be/src/runtime/io/data-cache.cc: http://gerrit.cloudera.org:8080/#/c/12987/1/be/src/runtime/io/data-cache.cc@79 PS1, Line 79: // Unlink the file so the disk space is recycled once the process exits. > I'm not sure how I feel about this - it could cause a lot of confusion havi Thanks for the feedback. I also considered the approach of cleaning up the directory on startup but this seems to make certain assumption about whether the directory specified by the user for caching is exclusively used by the Impalad process. While this is not a common scenario right now, I can see there may be configuration in the future where one may run multiple Impala containers on the same host and these containers may belong to the same cluster. So, wiping out a particular directory on restart isn't exactly safe. Another alternative would be to include the hostname / IP address + port number as a unique name for the file so we will automatically truncate the file on restart. However, there is no guarantee that an Impalad will restart with the same IP address / hostname after a crash, esp. in a public cloud setting. That said, one can also argue that this is a configuration issue and those Impala containers shouldn't be sharing directories etc. The feedback so far suggests that the "hidden" disk usage seems even more undesirable so may be wiping out the directory at startup is not as bad and we can just forbid sharing of caching directories by multiple Impalads on the same host. -- To view, visit http://gerrit.cloudera.org:8080/12987 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I734803c1c1787c858dc3ffa0a2c0e33e77b12edc Gerrit-Change-Number: 12987 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 11 Apr 2019 17:13:23 +0000 Gerrit-HasComments: Yes
