Todd Lipcon has posted comments on this change. Change subject: WIP: Refactor cache interface ......................................................................
Patch Set 1: (19 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 EvictionCall changed it to Cache::EvictionCallback and make null allows. Also changed the API to just take slice key/value now instead, which is a bit nicer Line 93: sizeof(key)), behavior); > bad indentation oh, this wasn't bad after all http://gerrit.cloudera.org:8080/#/c/2957/1/src/kudu/cfile/block_cache.h File src/kudu/cfile/block_cache.h: Line 58: class PendingEntry { > doc Done Line 68: *this = std::move(other); > I don't think move is necessary here, since it's already an rvalue ref I dont really understand why, but apparently it is. (without it, it tries to selected the deleted non-move assignment operator) Line 126: Release() > this doesn't need to be explicit since BlockCacheHandle will do so on destr Done 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 actually wasn't done, looked at the wrong thing. Will leave as is. 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 Done 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 Done 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 Done Line 60: JITWrapper* val = value.get(); > coudl do with some comment Done 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 Done Line 302: // key/value allocation. > above comment looks out of place a bit Done Line 455: If we > this makes it sound like there are some cases where we don't, but that's no Done 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 Done Line 110: // Because some cache implementations manage their own memory, and because > should probably say '(eg NVM)' here Done Line 118: data_size); > this is missing the data_size arg, but seems to have an extra data_size arg Done 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). Done Line 153: no longer needed, > evicted Done 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 n Done -- 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
