Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15011 )
Change subject: fs: move file cache to server ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/15011/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/15011/2/src/kudu/fs/log_block_manager.cc@727 PS2, Line 727: if (PREDICT_TRUE(block_manager_->file_cache_)) { : CONTAINER_DISK_FAILURE(block_manager_->file_cache_->DeleteFile(data_file_name), : data_failure_msg); : CONTAINER_DISK_FAILURE(block_manager_->file_cache_->DeleteFile(metadata_file_name), : metadata_failure_msg); : } else { : CONTAINER_DISK_FAILURE(block_manager_->env_->DeleteFile(data_file_name), : data_failure_msg); : CONTAINER_DISK_FAILURE(block_manager_->env_->DeleteFile(metadata_file_name), : metadata_failure_msg); : } Maybe, encapsulate this into the block manager itself? Or maybe have different types of block manager (caching and non-caching one)? http://gerrit.cloudera.org:8080/#/c/15011/2/src/kudu/server/server_base.h File src/kudu/server/server_base.h: http://gerrit.cloudera.org:8080/#/c/15011/2/src/kudu/server/server_base.h@108 PS2, Line 108: FileCache* file_cache() { return file_cache_.get(); } nit: add const specifier for the method? http://gerrit.cloudera.org:8080/#/c/15011/2/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/15011/2/src/kudu/server/server_base.cc@224 PS2, Line 224: TAG_FLAG(server_max_open_files, evolving); This flag is tagged 'evolving' by default, no? http://gerrit.cloudera.org:8080/#/c/15011/2/src/kudu/server/server_base.cc@227 PS2, Line 227: value < -1 >From the description of the flag, it seems to be that '-1' is a valid value >meaning 'auto-calculate the proper setting', no? http://gerrit.cloudera.org:8080/#/c/15011/2/src/kudu/server/server_base.cc@228 PS2, Line 228: Invalid max open files Maybe, include the name of the flag in the 'flagname' parameter to make the message more actionable? -- To view, visit http://gerrit.cloudera.org:8080/15011 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ice92c3622c954b06b773c58d51f08082010d7de3 Gerrit-Change-Number: 15011 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-Comment-Date: Mon, 13 Jan 2020 02:49:38 +0000 Gerrit-HasComments: Yes
