fix: change irrewritable attributes in config from atomic to primitive #352
Conversation
Walkthrough此次修改旨在简化和优化配置项的访问以及删除冗余代码。主要改动包括对各类配置访问的方式进行调整，移除了不必要的 Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/cmd_admin.cc (3 hunks)
- src/config.cc (5 hunks)
- src/config.h (7 hunks)
- src/db.cc (2 hunks)
- src/pikiwidb.cc (2 hunks)
- src/praft/praft.cc (4 hunks)
- src/replication.cc (2 hunks)
Additional comments not posted (19)
src/db.cc (1)
36-36: 确保配置项use_raft在初始化时进行了正确的设置和检查，以避免在运行时因配置不正确而引发的问题。Verification successful
确保配置项
use_raft在初始化时进行了正确的设置和检查，以避免在运行时因配置不正确而引发的问题。根据代码库中的信息：
src/config.cc中使用AddBool添加了use_raft配置项。src/config.h中将use_raft初始化为true。src/db.cc中有两处检查g_config.use_raft的代码。这些检查和设置表明
use_raft配置项在初始化时已正确设置和检查。
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 检查配置项 `use_raft` 是否在初始化时已经正确设置 rg --type cpp "use_raft" src/config.ccLength of output: 92
Script:
#!/bin/bash # 检查 `use_raft` 在代码库中的使用情况，以确保其在初始化时已正确设置和验证 rg --type cpp "use_raft" src/Length of output: 208
src/pikiwidb.cc (4)
145-145: 请确保线程数量不超过系统或应用程序的最大限制，以避免资源耗尽。Verification successful
线程数量限制已在代码中得到验证，确保不会超过系统或应用程序的最大限制。
src/pikiwidb.cc:
auto kMaxWorkerNum = IOThreadPool::GetMaxWorkerNum();if (num > kMaxWorkerNum) { ERROR("number of threads can't exceeds {}, now is {}", kMaxWorkerNum, num); }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 检查线程数设置是否超过了系统或应用程序的最大限制 rg --type cpp "kMaxWorkerNum" src/pikiwidb.ccLength of output: 210
155-155: 核实cmd_threads_.Init函数是否能够正确处理所有初始化失败的情况。Verification successful
cmd_threads_.Init函数的调用已经包含了初始化失败的处理逻辑。具体来说，如果status不是ok()，则会记录错误消息并返回false。
- 代码位置:
src/pikiwidb.cc，第 155 行Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 检查 `cmd_threads_.Init` 在初始化失败时的处理逻辑 ast-grep --lang cpp --pattern $'if ($$$) { cmd_threads_.Init($_, $_, $_) }' src/pikiwidb.ccLength of output: 92
Script:
#!/bin/bash # Locate the definition of `cmd_threads_.Init` ast-grep --lang cpp --pattern 'Init($_, $_, $_)' # Search for all instances of `cmd_threads_.Init` in the codebase rg 'cmd_threads_\.Init' --context 5Length of output: 675
267-267: 验证closeStd函数是否能够在所有标准文件描述符上正确执行，避免潜在的资源泄漏。Verification successful
closeStd函数已经在代码中正确实现并关闭了所有标准文件描述符（stdin、stdout、stderr），避免了潜在的资源泄漏。
closeStd函数中使用dup2函数将/dev/null文件描述符复制到标准输入、输出和错误文件描述符上。- 在
fd成功打开的情况下，确保了标准文件描述符被重定向到/dev/null，并且关闭了多余的文件描述符。代码位置：
src/pikiwidb.cc，第267行Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 检查 `closeStd` 函数是否在所有标准文件描述符上正确执行 rg --type cpp "closeStd" src/pikiwidb.ccLength of output: 81
Script:
#!/bin/bash # Extract the full implementation of the `closeStd` function from src/pikiwidb.cc ast-grep --lang cpp --pattern $'function closeStd() { $$$ }' src/pikiwidb.ccLength of output: 79
Script:
#!/bin/bash # List all function definitions in src/pikiwidb.cc to locate the `closeStd` function rg --type cpp "^[a-zA-Z_][a-zA-Z0-9_]*[ \t]*\([^)]*\)[ \t]*\{" src/pikiwidb.ccLength of output: 80
Script:
#!/bin/bash # Search for the `closeStd` function definition and its body in src/pikiwidb.cc rg --context 10 "closeStd" src/pikiwidb.ccLength of output: 690
140-141: 检查worker_threads_.Init函数调用是否正确处理了所有潜在的错误场景，特别是网络相关的错误。Verification successful
worker_threads_.Init函数在src/io_thread_pool.cc文件中定义，并且它确实处理了网络相关的错误场景。例如，如果base_.Listen失败，它会记录错误并返回false。因此，worker_threads_.Init函数调用已经正确处理了潜在的网络错误。
- 代码位置:
src/io_thread_pool.cc,bool IOThreadPool::InitScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 检查 `worker_threads_.Init` 在不同网络环境下的错误处理 ast-grep --lang cpp --pattern $'if ($$$) { worker_threads_.Init($_, $_, $_) }' src/pikiwidb.ccLength of output: 95
Script:
#!/bin/bash # Locate the definition of `worker_threads_.Init` and check for error handling within it ast-grep --lang cpp --pattern $'class $_ { $$$ Init($_, $_, $_) { $$$ } $$$ }' src/Length of output: 429
Script:
#!/bin/bash # Locate the `Init` method within the `worker_threads_` class and check for error handling rg --pcre2 --multiline --multiline-dotall 'class\s+\w+\s+{[^}]*\bInit\s*\([^)]*\)\s*{[^}]*}' src/Length of output: 99
Script:
#!/bin/bash # Locate the Init method within the worker_threads_ class and check for error handling rg --pcre2 --multiline --multiline-dotall 'worker_threads_.*\bInit\s*\([^)]*\)\s*{[^}]*}' src/Length of output: 976
Script:
#!/bin/bash # Extract the Init method implementation from src/io_thread_pool.cc sed -n '/bool IOThreadPool::Init/,/return true;/p' src/io_thread_pool.ccLength of output: 517
src/cmd_admin.cc (2)
63-63: 请检查路径操作是否考虑了跨平台的兼容性，特别是在不同的文件系统上。
118-118: 验证 IP 地址比较逻辑是否能够正确处理各种网络配置，包括 IPv6 地址。src/config.cc (3)
138-138: 检查所有配置项的引用是否在更新后仍然有效，以避免潜在的空指针或访问违规。Verification successful
所有配置项
daemonize,use_raft, 和rocksdb_enable_pipelined_write在代码中均有明确的初始化和使用，且没有发现潜在的空指针或访问违规问题。
daemonize在src/config.h中初始化为false，并在多个地方使用时进行了正确的检查。use_raft在src/config.h中初始化为true，并在使用时进行了检查。rocksdb_enable_pipelined_write在src/config.h中初始化为false，并在配置 RocksDB 选项时使用。因此，可以确认引用在更新后仍然有效。
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 检查配置项更新后的引用有效性 rg --type cpp "AddBool" src/config.ccLength of output: 244
Script:
#!/bin/bash # 检查配置项 daemonize, use_raft, rocksdb_enable_pipelined_write 的引用有效性 rg --type cpp "daemonize" rg --type cpp "use_raft" rg --type cpp "rocksdb_enable_pipelined_write"Length of output: 887
Script:
#!/bin/bash # 检查 daemonize, use_raft, rocksdb_enable_pipelined_write 变量的初始化和使用情况 rg --type cpp "daemonize" --context 3 rg --type cpp "use_raft" --context 3 rg --type cpp "rocksdb_enable_pipelined_write" --context 3Length of output: 5536
75-81: 确保布尔值设置逻辑在不同的语言环境下都能正确解析 "yes" 和 "no"。
121-121: 请确认数字值设置逻辑能够正确处理所有边界条件和异常输入。src/config.h (5)
36-37: 改变了BaseValue构造函数的参数，从之前的字符串参数变为了直接使用std::string对象，这有助于减少不必要的字符串拷贝。同时，增加了CheckFunc类型的参数，以支持更灵活的值检查逻辑。
85-100:NumberValue模板类在构造函数中添加了对值范围的检查（min和max），这是一个很好的实践，可以在运行时防止设置不合理的数值。同时，这里移除了原子操作，使用了普通的指针，这可能是为了配置系统的简化和性能优化。同样，需要关注这一改变是否会影响线程安全。
104-114:BoolValue类的实现现在使用普通的布尔指针代替原来的原子布尔类型。这一改变应该是基于性能考虑。然而，需要验证在多线程环境下，这种改动是否可能导致数据竞争。
65-68: 在StringValue类中，value_ptr_vec参数类型从std::vector<AtomicString*>更改为std::vector<std::string*>，这表明不再使用原子操作来管理字符串值，可能是为了简化设计或提高性能。需要确保这一改变不会引入线程安全问题。
45-45:Set方法的参数从force改为init_stage，这看起来是为了更清晰地表达参数的意图，即这个设置是否在初始化阶段进行。建议在实际使用中验证这一变更是否符合所有调用场景的预期。src/replication.cc (2)
178-178: 在这里增加了对主节点地址自身的检查，如果配置的主节点地址是自身的地址，将触发断言。这是一个重要的安全检查，可以防止配置错误导致的循环复制问题。建议在文档中明确指出配置主节点地址时应避免指向自身。
228-232: 这段代码负责发送replconf命令，用于配置复制过程中的监听端口。这是复制设置中的一个重要步骤，确保了节点间正确的通信配置。建议在实际部署时进行详细的测试，以确保所有节点都能正确处理这一命令。src/praft/praft.cc (2)
123-125: 这里的代码负责将配置的IP地址和端口转换为butil::ip_t类型，并检查转换是否成功。这是网络编程中常见的错误点，良好的错误处理可以防止因地址解析错误导致的服务启动失败。建议增加更详细的日志输出，帮助定位问题。
153-153: 在这里设置了Raft日志、元数据和快照的存储路径，这些配置对于Raft协议的稳定运行至关重要。建议在系统部署前进行充分的测试，确保这些路径的读写权限和磁盘空间都符合要求。
| inline void AddString(const std::string& key, bool rewritable, const std::vector<std::string*>& values_ptr_vector) { | ||
| config_map_.emplace(key, std::make_unique<StringValue>(key, nullptr, rewritable, values_ptr_vector)); | ||
| } | ||
| inline void AddStrinWithFunc(const std::string& key, const CheckFunc& checkfunc, bool rewritable, | ||
| std::vector<AtomicString*> values_ptr_vector) { | ||
| const std::vector<std::string*>& values_ptr_vector) { |
There was a problem hiding this comment.
Tip
Codebase Verification
AddString 和 AddStrinWithFunc 方法在添加键值对时没有检查键是否已存在于 config_map_ 中。为了确保没有重复添加相同的键，建议在添加新键之前先检查 config_map_ 中是否已存在该键。
src/config.hAddString方法在添加键值对时没有进行键存在性检查。AddStrinWithFunc方法在添加键值对时没有进行键存在性检查。
请在这些方法中添加键存在性检查，以避免重复添加相同的键。
Analysis chain
添加了AddString和AddStrinWithFunc两个方法，用于简化配置项的添加。这些方法通过直接操作内部的config_map_来注册新的配置项，使得代码更加简洁。需要检查这些方法的使用是否正确处理了所有潜在的异常情况，例如重复添加相同的键。
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查AddString和AddStrinWithFunc方法的调用情况，确保没有重复添加相同的键。
rg --type cpp "AddString" -A 3
Length of output: 1121
Script:
#!/bin/bash
# Check if config_map_ is checked for existing keys before adding new entries in AddString and AddStrinWithFunc methods.
rg --type cpp "config_map_" -A 5
Length of output: 4153
There was a problem hiding this comment.
这些方法只会在PConfig::PConfig()中被调用，可以明确保证没有重复的键，所以不需要额外的检查。
There was a problem hiding this comment.
@mirthfulLee, thank you for clarifying the usage context of the AddString and AddStrinWithFunc methods within the PConfig::PConfig() constructor. Based on your insight, it seems these methods are used in a controlled manner that ensures no duplicate keys are added. I'll take note of this for future reviews and appreciate your detailed understanding of the codebase's structure.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the usage of `AddString` and `AddStrinWithFunc` methods within the `PConfig::PConfig()` constructor to verify the user's claim.
ast-grep --lang cpp --pattern $'class PConfig {
PConfig() {
$$$
}
}'
Length of output: 77
Learnings added
User: mirthfulLee
PR: OpenAtomFoundation/pikiwidb#352
File: src/config.h:238-242
Timestamp: 2024-06-15T08:37:03.282Z
Learning: The `AddString` and `AddStrinWithFunc` methods are only called within the `PConfig::PConfig()` constructor, ensuring no duplicate keys are added.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| @@ -107,7 +107,7 @@ void DB::LoadDBFromCheckpoint(const std::string& checkpoint_path, bool sync [[ma | |||
| storage_options.options.ttl = g_config.rocksdb_ttl_second.load(std::memory_order_relaxed); | |||
| storage_options.options.periodic_compaction_seconds = | |||
| g_config.rocksdb_periodic_second.load(std::memory_order_relaxed); | |||
| if (g_config.use_raft.load(std::memory_order_relaxed)) { | |||
| if (g_config.use_raft) { | |||
There was a problem hiding this comment.
Tip
Codebase Verification
请确保在使用 RAFT 功能前，已经正确初始化和配置了相关的参数，以避免在运行时出现错误。当前代码中没有明确的初始化和验证步骤。
src/db.cc中直接使用了g_config.use_raft。src/config.cc中添加了use_raft配置。src/config.h中声明了use_raft变量。
建议在使用 use_raft 前增加初始化和验证步骤。
Analysis chain
请确保在使用 RAFT 功能前，已经正确初始化和配置了相关的参数，以避免在运行时出现错误。
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查 RAFT 相关配置是否已经在启动前被正确设置
rg --type cpp "use_raft" src/config.cc
Length of output: 92
Script:
#!/bin/bash
# 检查 RAFT 配置是否在使用前被正确初始化和验证
rg --type cpp "use_raft" src/ | grep -E 'init|validate|check'
Length of output: 61
Script:
#!/bin/bash
# 搜索代码库中所有与 use_raft 相关的出现和使用情况
rg --type cpp "use_raft" src/
Length of output: 208
There was a problem hiding this comment.
g_config.use_raft从atomic<bool>更新为了原生的bool，因此不需要load就可以直接使用了。因为该属性是不可修改的，所以并没有并发问题。
There was a problem hiding this comment.
@mirthfulLee, 感谢您的澄清和详细说明！理解了g_config.use_raft从atomic<bool>更新为原生bool的变化，以及由于其不可修改性质，确实没有并发问题。如果有其他需要帮助的地方，请随时告诉我！
panlei-coder
requested review from
AlexStocks,
panlei-coder and
dingxiaoshuai123
| if (pstd::StringEqualCaseInsensitive(value, "yes")) { | ||
| *value_ = true; | ||
| } else { | ||
| *value_ = false; |
There was a problem hiding this comment.
在value_还是std::atomic*类型的情况下，还是使用value_->store(true);和value_->store(false);比较好
| std::atomic<size_t> databases = 16; | ||
| std::atomic_uint32_t worker_threads_num = 2; | ||
| size_t databases = 16; | ||
| uint32_t worker_threads_num = 2; | ||
| std::atomic_uint32_t slave_threads_num = 2; | ||
| std::atomic<size_t> db_instance_num = 3; |
There was a problem hiding this comment.
这两个是动态可调整嘛？ 我在issue里看到说db_instance_num也是只读，这个需要考虑改成普通变量吗
The config has been modified extensively in PR, and many variables have been changed to non-atomic. The modification may cause problems. Moreover, this case cannot provide test cases. It is recommended to perform a round of stress testing first, and then merge if there is optimization. |
| std::atomic<size_t> databases = 16; | ||
| std::atomic_uint32_t worker_threads_num = 2; | ||
| size_t databases = 16; | ||
| uint32_t worker_threads_num = 2; | ||
| std::atomic_uint32_t slave_threads_num = 2; | ||
| std::atomic<size_t> db_instance_num = 3; |
There was a problem hiding this comment.
是的，加载之后不会修改，slave_threads_num 也是
There was a problem hiding this comment.
像上面的 master_ip 逻辑上应该也不会在加载之后修改，可以对上面的 config 类中的变量逐个考虑一下是否是只读属性
Basically fixed issue #329
Summary by CodeRabbit