Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19327 )
Change subject: [Tool] KUDU-3318 Compact container metadata file manually ...................................................................... Patch Set 9: (19 comments) http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.h@215 PS8, Line 215: static Status GetContainerNames(Env* env, const std::string& data_dir, > Add some comments for this function. Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.cc@957 PS8, Line 957: // Aborted: the container will be repaired later; > It's similar to LogBlockContainerNativeMeta::ProcessRecords(), how about re Using LogBlockContainerNativeMeta::ProcessRecords() needs to open log block manager. But this tool can not open log block manager, because opening log block manager will load all log block containers and causes staring tserver very slowly, which is the problem this tool needs to solve. http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.cc@2789 PS8, Line 2789: > Just skip the compact procedure if compact_metadata is 0. Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.cc@2801 PS8, Line 2801: > I guess the .compacted is needed to be deleted if the .offset file generate In function GenerateCompactedFile(), there exists a method to delete .compacted file, if generating it failed. Besides, on the next operation 'merge', it will check .compacted file and .offset file existing at the same time. http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.cc@2930 PS8, Line 2930: optional<int64_t> limit; : if (FLAGS_log_container_max_blocks == -1) { : // No limit, unless this is KUDU-1508. : : // The log block manager requires hole punching and, of the ext : // filesystems, only ext4 supports it. Thus, if this is an ext : // filesystem, it's ext4 by definition. : if (buggy_el6_kernel_ && dd->fs_type() == FsType::EXT) { : uint64_t fs_block_size = : dd->instance()->metadata()->filesystem_block_size_bytes(); : bool untested_block_size = : !ContainsKey(kPerFsBlockSizeBlockLimits, fs_block_size); : string msg = Substitute( : "Data dir $0 is on an ext4 filesystem vulnerable to KUDU-1508 " : "with $1block size $2", dd->dir(), : untested_block_size ? "untested " : "", fs_block_size); : if (untested_block_size) { : LOG(WARNING) << msg; : } else { : > There are a bit of duplicate, how about adding the suffixes to a list, and Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.cc@2987 PS8, Line 2987: this->OpenDataDir(dd_raw, results, s, containers_processed, containers_total); > How about do a small refactor for LogBlockManager::RewriteMetadataFile() an RewriteMetadataFile() is only called with log block manager opened. Even we can make this function to a static function, but the macro `RETURN_NOT_OK_LBM_DISK_FAILURE_PREPEND` can not be a static one, and the macro is important. http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/fs/log_block_manager.cc@3028 PS8, Line 3028: dead_containers.c > There is a similar code piece in LogBlockManager::OpenDataDir(), is it poss Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@151 PS8, Line 151: "for every container." : "--compact: Compact metadata f > nit: use a std::set/std::unordered_set for fast lookup, and JoinStrings can Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1036 PS8, Line 1036: > Why it's 1 at the beginning? The same to cur_finished_containers_count and It uses 'fetch_add' to keep thread-safe. 'fetch_add' will return the old value. So set the value to 1 at the beginning. http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1033 PS8, Line 1033: unique_ptr<ThreadPool> pool; : RETURN_NOT_OK(ThreadPoolBuilder("metadata-compact-pool") : .set_max_threads(FLAGS_num_threads) : .Build(&pool)); : > nit: Move them closer to the place where use it. Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1040 PS8, Line 1040: > seen_fatal_er > nit: auto Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1050 PS8, Line 1050: tainers += cont > nit: auto Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1056 PS8, Line 1056: if (seen_fatal_error.load()) { > The container_names has been obtained in line 1043, how about reuse it? It can not be reused. Every data directory needs to get container names separately. http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1061 PS8, Line 1061: unordered_set<string> container_names; : Status s = LogBlockManager::GetContainerNames(env, dd->dir(), &container_names); : if (!s.ok()) { > While the tserver is running, the container_names get in previous step may OK, delete this piece of codes. http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1091 PS8, Line 1091: > Why skip only when execute "merge" operation? If merge operation fails, it has no affect, just skip it. TServer will use the old metadata file to start the progress. I will update the comments. http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1102 PS8, Line 1102: p, FLAGS_metada > There is just an INFO log even if there is an error occurred ? OK, delete these logs. http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1113 PS8, Line 1113: ed_dir > For the container failed to process, is the result correct and meaningful t Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1132 PS8, Line 1132: total > It is in expect to consider the directory "finished" if skipped from line 1 Done http://gerrit.cloudera.org:8080/#/c/19327/8/src/kudu/tools/tool_action_fs.cc@1297 PS8, Line 1297: _data_dirs") : .AddOptionalParameter("fs_metadata_dir") : .AddOptionalParameter("fs_wal_dir") : .AddOptionalParameter("metadata_op_type") : .AddOptionalParameter("num_threads") : .AddOptionalParameter("log_container_live_metadata_before_compact_ratio") : .Build(); > It would be more helpful if moving these descriptions to metadata_op_type g Done -- To view, visit http://gerrit.cloudera.org:8080/19327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id61cb6c88d7a093a8448e2497639f5c5c094efa4 Gerrit-Change-Number: 19327 Gerrit-PatchSet: 9 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Fri, 02 Feb 2024 06:25:57 +0000 Gerrit-HasComments: Yes
