Todd Lipcon has posted comments on this change. Change subject: WIP: Refactor cache interface ......................................................................
Patch Set 1: (20 comments) http://gerrit.cloudera.org:8080/#/c/2957/1/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: Line 51: // TODO: nothing to do? we should support NULL deleter, and perhaps change the name to EvictionCallback? Line 93: sizeof(key)), behavior); bad indentation http://gerrit.cloudera.org:8080/#/c/2957/1/src/kudu/cfile/block_cache.h File src/kudu/cfile/block_cache.h: Line 48: struct CacheKey { doc Line 58: class PendingEntry { doc Line 68: *this = std::move(other); I don't think move is necessary here, since it's already an rvalue ref Line 126: Release() this doesn't need to be explicit since BlockCacheHandle will do so on destruction Line 129: // TODO: change BlockCacheHandle to implement a move constructor and return it here : // instead of using an out-param. this is done now, should change Line 143: : // NOTE: The returned pointer may either be passed to Insert() or Free(). : // It must NOT be freed using free() or delete[]. no longer returns a pointer. above docs should be updated http://gerrit.cloudera.org:8080/#/c/2957/1/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: Line 291: public: // TODO fix http://gerrit.cloudera.org:8080/#/c/2957/1/src/kudu/codegen/code_cache.cc File src/kudu/codegen/code_cache.cc: Line 38: // void* values. To delete, we just release our shared ownership. update: not void* value Line 60: JITWrapper* val = value.get(); coudl do with some comment http://gerrit.cloudera.org:8080/#/c/2957/1/src/kudu/util/cache.cc File src/kudu/util/cache.cc: Line 176: Cache methods Cache::Lookup Line 302: // key/value allocation. above comment looks out of place a bit Line 455: If we this makes it sound like there are some cases where we don't, but that's not the case http://gerrit.cloudera.org:8080/#/c/2957/1/src/kudu/util/cache.h File src/kudu/util/cache.h: Line 48: // chunk which starts with the key so we delete that. this doc isn't very clear Line 110: // Because some cache implementations manage their own memory, and because should probably say '(eg NVM)' here Line 118: data_size); this is missing the data_size arg, but seems to have an extra data_size arg on the line below Line 139: virtual PendingHandle* Allocate(Slice key, int val_len, int charge) = 0; docs should say what happens to 'key' (copied vs has to stay valid, etc). should also be clear that allocating for a given key does _not_ make that key visible to other threads doing a Lookup Line 153: no longer needed, evicted http://gerrit.cloudera.org:8080/#/c/2957/1/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 542: handle->charge = charge; the charge should probably include the val_len since it's coming from the nvm address space (per comment in removed code) -- To view, visit http://gerrit.cloudera.org:8080/2957 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iedeb24c0233bf062a0a6450a9fd3a7c57499144f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
