Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14988 )
Change subject: file cache: unify across file types ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache-test.cc File src/kudu/util/file_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache-test.cc@374 PS2, Line 374: TEST_F(MixedFileCacheTest, TestBothFileTypes) { Does it make sense to add a test to verify that the following is kept true, as claimed in file_cache.h The same file may be opened simultaneously as different descriptor types; the instances will share nothing ? http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache.h File src/kudu/util/file_cache.h: http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache.h@127 PS2, Line 127: Status OpenExistingRWFile(const std::string& file_name, : std::shared_ptr<RWFile>* file); : Status OpenExistingRandomAccessFile(const std::string& file_name, : std::shared_ptr<RandomAccessFile>* file); Would a templatized method be a better choice here? http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache.h@210 PS2, Line 210: const char* nit: const char* const ? http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache.cc File src/kudu/util/file_cache.cc: http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache.cc@487 PS2, Line 487: desc Does it make sense to add DCHECK(desc) here? http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache.cc@510 PS2, Line 510: desc ditto http://gerrit.cloudera.org:8080/#/c/14988/2/src/kudu/util/file_cache.cc@625 PS2, Line 625: descs readability nit here and elsewhere: maybe, rename it into 'descriptors' since 'descs' and 'descs' are harder to tell apart. -- To view, visit http://gerrit.cloudera.org:8080/14988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e5cf6cadb7f0d45492af8b8a7541853d587409c Gerrit-Change-Number: 14988 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jan 2020 19:41:34 +0000 Gerrit-HasComments: Yes
