Adar Dembo 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 diffe This pattern only takes place in one place; I don't think it's worth any encapsulation. 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? Done 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? I just copied all this stuff from block_manager.cc as is. Done. 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 Yes; here we're forbidding values less than -1 (like -2, -3, etc.). But this seems unnecessarily complicated; I'll make 0 the magic value and change the flag to be unsigned. 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 Moot, no more validator. -- 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: 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 21:50:53 +0000 Gerrit-HasComments: Yes
