Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15081 )
Change subject: file cache: support alternate open modes ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/15081/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/15081/1/src/kudu/fs/log_block_manager.cc@805 PS1, Line 805: } while (PREDICT_FALSE(metadata_status.IsAlreadyPresent() || : data_status.IsAlreadyPresent())); > Is it possible this cycle run infinitely if the file got some sort of fs-le It could loop forever if for some reason every new data/metadata file we tried to create already existed on the filesystem. That could happen if the ObjectIdGenerator stopped producing random UUIDs (i.e. if there was a bug in it). But I don't think a broken filesystem would yield such behavior. In any case, this isn't any more or less likely to happen than it was before, right? http://gerrit.cloudera.org:8080/#/c/15081/1/src/kudu/util/file_cache.h File src/kudu/util/file_cache.h: http://gerrit.cloudera.org:8080/#/c/15081/1/src/kudu/util/file_cache.h@125 PS1, Line 125: Every mode tries to behave as a POSIX filesystem would, but the : // semantics aren't 100% right when using the more "interesting" modes (such : // as CREATE_OR_OPEN_WITH_TRUNCATE) and deleting files concurrently. Beware. > It it worth enumerating examples of how it differs? Also if it's not used a Yeah, I guess I can get rid of CREATE_OR_OPEN_WITH_TRUNCATE since we won't be using it. There's still latent weirdness though. For example, open(CREATE_OR_OPEN) + delete + open(CREATE_OR_OPEN) (without a close() in between). Should we "undelete" the file? If so, we should probably also truncate it, as that's how it'd appear if done by the filesystem. I'm fairly confident that for the use cases we care about, this will work. But I haven't thought through all of the combinations and how their semantics compare with that of a POSIX filesystem. Meaning, the comment I'm leaving here is intended to warn folks who think this is a general purpose tool with full POSIX filesystem semantics: if you want to use the file cache in some new way, make sure you think through the semantics you need and whether there are gaps. http://gerrit.cloudera.org:8080/#/c/15081/1/src/kudu/util/file_cache.cc File src/kudu/util/file_cache.cc: http://gerrit.cloudera.org:8080/#/c/15081/1/src/kudu/util/file_cache.cc@340 PS1, Line 340: out > nit: maybe rename the variable to 'existing_file' then so that's a bit more The templating suggestion you offered makes this "double duty" unnecessary, so I'll leave the name the way it is. http://gerrit.cloudera.org:8080/#/c/15081/1/src/kudu/util/file_cache.cc@360 PS1, Line 360: // TODO(adar): this doesn't need to be a member as it's only used by Init, but : // there's no easy way to plumb it through KuduOnceDynamic. > I wonder if it would help if we were to templatize ReopenFileIfNecessary wi Yeah I need to change this; the crappy hack I used was racy. Your suggestion generally works, but C++ doesn't allow partial template specialization for functions, so there's some code duplication. Take a look and let me know what you think. http://gerrit.cloudera.org:8080/#/c/15081/1/src/kudu/util/file_cache.cc@557 PS1, Line 557: DCHECK(!FindDescriptorUnlocked(file_name, FindMode::DONT_CREATE, : &rwf_descs_, &ignored)); > nit: wrap into #ifndef NDEBUG ... #endif as for the OpenFile(..., shared_pt Done -- To view, visit http://gerrit.cloudera.org:8080/15081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie167302ef85b8e1a40fbb89a7742e2cbb43bcec3 Gerrit-Change-Number: 15081 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 22 Jan 2020 17:02:48 +0000 Gerrit-HasComments: Yes
