Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15081 )
Change subject: file cache: support alternate open modes ...................................................................... Patch Set 1: (3 comments) 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 at all, would you be against punting on supporting that mode entirely? Especially if it's not used anywhere in our codebase. Eg by DCHECKing unsupported inputs. 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 obvious? 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 with the OpenMode? And then call it with <initial_mode> in InitOnce(). Though punting is fine too. -- 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 <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 21 Jan 2020 21:42:14 +0000 Gerrit-HasComments: Yes
