Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/18191 )
Change subject: IMPALA-9433: Improved caching of HdfsFileHandles ...................................................................... Patch Set 21: (8 comments) > (12 comments) > > > > > (8 comments) > > > > > > > > Thank you for taking this on. There is a lot of history here. > > > > Originally, the file handle cache used a generic structure > like > > > > this: > > > > https://github.com/apache/impala/blob/branch-2.10.0/be/src/util/lru-cache.h > > > > > > > > In my rewrite, I switched it to remove the generic structure. > > > This > > > > heads back in the other direction. > > > > > > > > I like that you have backend tests for the generic data > > > structure, > > > > which is definitely one advantage of that approach. One > > question > > > I > > > > have about moving back to a generic structure is whether we > > would > > > > be able to add new customization to the file handle cache > case. > > I > > > > had been thinking about adding a file structure that could > > > contain > > > > additional per-file data and/or stats. Is that possible with > > the > > > > new generic structure? > > > > > > "had been thinking about adding a file structure that could > > contain > > > additional per-file data and/or stat" > > > > > > I was thinking about similar things (e.g. caching processed > > > Parquet/ORC headers), but this seems a somewhat different > feature > > > to me - while we want to cache more than one file handle per > file > > > and apply LRU logic per handle, we want to cache data for a > file > > > only once and apply LRU logic per file. > > > > Yeah, it's unclear whether we would ever want to extend the file > > handle cache > > to deal with other things. Separate data structures may be > cleaner > > even > > if it means duplicating filename strings or other things. The > file > > handle > > cache is pretty unusual in structure and historically we haven't > > extended > > it. I don't have any strong objection to a generic structure. I > > just wanted > > to think through whether there are any extensions that would end > up > > getting more complicated. > > > > For more ordinary caches that don't need duplication, we should > be > > using the > > cache implementations in be/src/util/cache, because that also > gets > > us different > > cache eviction policies. > > Moving the caching to a separate class definitely helps with > testing. > > I think making it generic helps with encapsulation too, as the > caching algorithm (even if it is somewhat specialized to a use > case) has nothing to do with the stored data. Moreover, making it > generic helped with unit testing too, as I didn't have to juggle > around file handles to test out the caching feature. > > As I understand, the requirements were simple enough to fit it in a > generic structure. This would help in the future to decide - if we > ever want to use something similar to this - whether if it's easier > to extend with some configurability or not. We can specialize this > more towards file handle caching, although the arguments for > templated key/value are still applicable in that case, maybe we can > change the name and place of the code for something less generic. > > Regarding to per-file data/stats, we could squeeze it in (e.g. > unordered_map<string, pair<Data, list<Handler>> but I don't think > that is a good direction, it breaks the encapsulation of the cache. > I would rather put a container in FileHandleCache class next to the > cache and do handle based operations/metrics during creation > (GetFileHandle() new entry branch) and do access based > operation/metrics in FileHandleCache::Accessor This generic design is good, and we don't need to overthink this. My feeling is that the odds of us reusing the generic structure for something else are pretty low. (We'll find out! Maybe I'm wrong.) If you believe that, then I think an intermediate point is a non-generic structure that allows mocking the FileHandle. In other words, it allows for writing backend tests without creating real file handles, but it also isn't dealing with arbitrary types. Either way, I think if we need to extend this, then we will find a way to make it work. I'm not concerned. http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/runtime/io/handle-cache.inline.h File be/src/runtime/io/handle-cache.inline.h: http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/runtime/io/handle-cache.inline.h@167 PS21, Line 167: // Opening a file handle requires talking to the NameNode so it can take some time. : RETURN_IF_ERROR(accessor_tmp.Get()->Init(hdfs_monitor_)); > Yes, it will be in_use, does not lock the cache and not available for other Ok, great! http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.h File be/src/util/lru-multi-cache.h: http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.h@170 PS21, Line 170: Container_internal& container; : typename Container_internal::iterator it; > Something has to own the objects. The point of unordered_map<string, list> I was thinking about whether there is a way to avoid having the element keep a link to itself. Intrusive lists don't require that, but then you need to figure out ownership. Implementing ownership yourself is doable, but it probably isn't worth it. If the Accessor stored an iterator to the std::list element for the ValueType_internal rather than a pointer, then I think the self-link wouldn't be necessary. For std::list, iterators are not invalidated unless the element itself is deleted, so it is safe to save this. The iterator provides direct access to the ValueType_internal as well. We'd need to figure out how to initialize an empty Accessor, but that seems solvable. http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.h@252 PS21, Line 252: ValueType_internal* p_value_internal_; Consider making this an iterator to the std::list element http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.inline.h File be/src/util/lru-multi-cache.inline.h: http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.inline.h@147 PS21, Line 147: // Move the object to the back of the owning list as it is no longer available : container.splice(container.end(), container, container_it); > No other reason, just what you mentioned. With moving the mtime to the key, Ok, that makes sense. http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.inline.h@175 PS21, Line 175: container.emplace_back((*this), stored_key, container, std::forward<Args>(args)...); If you wanted to get an iterator back, the regular emplace() call returns an iterator to the new element. So, if you do emplace(container.end(), Args...) that would get the iterator to the element added. http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.inline.h@201 PS21, Line 201: // Move the object to the front, keep LRU relation in owning list too to : // be able to age out unused objects : container.splice(container.begin(), container, p_value_internal->it); > I agree that moving during get is not necessary, but I think it is useful d Basically, once you decide to move the in_use=true to the end of the list in Get(), then you also need to move things here. This makes sense. http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.inline.h@222 PS21, Line 222: if (container.size() == 1) { : // Last element, owning list can be removed to prevent aging : hash_table_.erase(p_value_internal->key); : } else { : // Remove from owning list : container.erase(p_value_internal->it); : } > At this point, the element is in use, not in the LRU list. It gets unlinked Ok, so can we add a DCHECK that this is in use? Maybe also add a comment that it is not on the LRU list. http://gerrit.cloudera.org:8080/#/c/18191/21/be/src/util/lru-multi-cache.inline.h@251 PS21, Line 251: // Remove from LRU cache : value_internal.member_hook.unlink(); Nit: Can we add a DCHECK that this is not in use? -- To view, visit http://gerrit.cloudera.org:8080/18191 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6b5c5e9e2b5db2847ab88c41f667c9ca1b03d51a Gerrit-Change-Number: 18191 Gerrit-PatchSet: 21 Gerrit-Owner: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 23 Feb 2022 19:33:16 +0000 Gerrit-HasComments: Yes
