helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: LBM moves half-present containers out of the way ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager-test.cc@1911 PS1, Line 1911: MetricRegistry new_registry; : scoped_refptr<MetricEntity> new_entity = : METRIC_ENTITY_server.Instantiate(&new_registry, "test"); > Can't you reuse 'entity' and 'registry' below? Well, the 'registry' can be reused, but 'entity' can't. The reason is that there has been values in 'entity' already. And i think a new entity will be better. http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@755 PS1, Line 755: // Check that both the metadata and data files exist and have valid lengths. > It would be good to decompose this entire section into a separate function hmm, maybe a new static function is appropriate. http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774 PS1, Line 774: // Handle a half-present container whose data file is missing. Open > What about half-present containers whose metadata file is missing? I can't In the destructor of 'LogBlockContainer', the data file is deleted before metadata file, so basically there is no case where the metadata file has gone missing. Indeed, i came across a case where a data file went missing, but metadata not yet. I think manual intervertion is necessary when the metadata file is missing in case, because it is not a common case. http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@779 PS1, Line 779: set<BlockId> live_blocks; > An unordered_set would be more efficient for this use case. Done http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@780 PS1, Line 780: unique_ptr<RandomAccessFile> metadata_reader; : RETURN_NOT_OK_CONTAINER_DISK_FAILURE(block_manager->env()->NewRandomAccessFile( : metadata_path, &metadata_reader)); : ReadablePBContainerFile pb_reader(std::move(metadata_reader)); : RETURN_NOT_OK_CONTAINER_DISK_FAILURE(pb_reader.Open()); > Please consolidate this with the equivalent code in LogBlockManager::Proces I did consider using the equivalent code from 'LogBlockManager::ProcessRecords'earlier. But there are two reasons why not: 1) the code in 'LogBlockManager::ProcessRecords' is much more complicated and some of them are needless for checking here; 2) more importantly, the 'LogBlockContainer::Open' is a static function and 'LogBlockManager::ProcessRecords' not; http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@801 PS1, Line 801: if ((read_status.IsEndOfFile() && live_blocks.empty())) { > What if read_status is not EndOfFile? Seems like that'll cause us to procee Our initial goal was to cover scenarios when the data file has gone missing. In particular, an exception scenario occurs when the container is being deleted, but this does not include a operation system crash(which needs manual intervention). And only an operating system crash would cause half a record(a partial trailing record). So there is no need to notice that the read_status is not EndOfFile. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Mon, 21 Jan 2019 00:48:45 +0000 Gerrit-HasComments: Yes
