feat: Add log size calculation by index range #10
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe pull request introduces a new private member variable Changes
Sequence DiagramsequenceDiagram
participant Node
participant NodeImpl
participant LogManager
Node->>NodeImpl: get_log_size_diff_by_index(index1, index2)
NodeImpl->>LogManager: Retrieve log entries
LogManager-->>NodeImpl: Return log entries
NodeImpl->>NodeImpl: Calculate total log size
NodeImpl-->>Node: Return log size difference
Poem
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 4
🧹 Nitpick comments (3)
test/test_node.cpp (2)
3923-3929: Consider making log sizes explicit for better test maintainability.The test data setup could be more explicit about the expected sizes of each log entry.
const std::vector<std::string> test_logs = { - "log1", // 4 bytes - "longer_log2", // 11 bytes - "log3", // 4 bytes - "very_very_long_log4" // 19 bytes + "log1", // size = 4 bytes + "longer_log2", // size = 11 bytes + "log3", // size = 4 bytes + "very_very_long_log4" // size = 19 bytes }; -const int expected_total_size = 4 + 11 + 4 + 19; +// Total size of all log entries +const int expected_total_size = 4 + 11 + 4 + 19; // 38 bytes
3942-3943: Add clarifying comment about initial configuration log.The test assumes knowledge about an initial configuration log. This should be explicitly documented.
const int64_t last_idx = leader->get_last_log_index(); -ASSERT_EQ(last_idx, test_logs.size() + 1); // Initial config log + 4 test logs +// last_idx should be test_logs.size() + 1 because: +// - Index 1: Initial configuration log (automatically added) +// - Index 2-5: Our 4 test log entries +ASSERT_EQ(last_idx, test_logs.size() + 1);src/braft/node.cpp (1)
2276-2288: Consider optimizing memory management and logging.
- The log entry memory management could be improved by using RAII patterns.
- The INFO level logging for each entry size might be too verbose for production.
Consider these improvements:
- LogEntry* entry = _log_manager->get_entry(i); - if (!entry) { - LOG(ERROR) << "node " << _group_id << ":" << _server_id - << " failed to get log entry at index=" << i; - return -1; - } - LOG(INFO) << "node entry index " << i << " : size " << entry->data.length(); - - total_size += entry->data.length(); - entry->Release(); + std::unique_ptr<LogEntry> entry(_log_manager->get_entry(i)); + if (!entry) { + LOG(ERROR) << "node " << _group_id << ":" << _server_id + << " failed to get log entry at index=" << i; + return -1; + } + BRAFT_VLOG << "node entry index " << i << " : size " << entry->data.length(); + + total_size += entry->data.length();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/braft/log_manager.h(1 hunks)src/braft/node.cpp(1 hunks)src/braft/node.h(1 hunks)src/braft/raft.cpp(1 hunks)src/braft/raft.h(1 hunks)test/test_node.cpp(1 hunks)
🔇 Additional comments (4)
test/test_node.cpp (1)
3945-3962: Well-structured test cases covering key scenarios.The test cases comprehensively cover:
- Same index calculation
- Full range calculation
- Reverse index order
- Partial range calculation
- Out-of-bounds detection
The test implementation is thorough and well-organized.
src/braft/node.cpp (3)
2255-2259: LGTM! Parameter validation is handled correctly.The implementation correctly handles the case where index1 > index2 by swapping them to ensure proper range calculation.
2260-2274: LGTM! Range validation is thorough and provides clear error messages.The code properly validates that both indices are within the valid range of log entries, checking against both first_log_index and last_log_index.
2290-2291: LGTM! Return value is appropriate.The method correctly returns the total size or -1 on error.
Comment on lines
+156
to
+157
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize and document thread safety for the new member variable.
The new _virtual_first_log_id member variable needs:
- Initialization in the constructor
- Documentation about thread safety and synchronization with
_last_snapshot_id - Validation logic to ensure no gaps with
_last_snapshot_id
Comment on lines
+275
to
+277
| int Node::get_log_size_diff_by_index(int index1, int index2) { | ||
| return _impl->get_log_size_diff_by_index(index1, index2); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and documentation.
The new method needs:
- Parameter validation for index1 and index2
- Documentation about:
- Valid index ranges
- Return value meaning
- Error cases
- Error handling for invalid indices
Example documentation:
// Calculate the total size of log entries between index1 and index2 (inclusive).
// Returns:
// >0: Total size in bytes
// -1: If indices are invalid or out of range
// -2: If index2 < index1
// Note: Both indices must be within the valid log range [first_log_index, last_log_index]
src/braft/node.h
Outdated
Comment on lines
266
to
267
| int get_log_size_diff_by_index(int index1, int index2); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add method documentation.
Add documentation to describe:
- Method purpose
- Parameter constraints
- Return value semantics
- Thread safety guarantees
Example:
// Calculate the total size of log entries between index1 and index2 (inclusive).
// Thread-safe.
// @param index1 Start index, must be >= first_log_index
// @param index2 End index, must be <= last_log_index and >= index1
// @return Total size in bytes if successful, negative value on error
There was a problem hiding this comment.
@AlexStocks Here's the documented version of the method:
// Calculate the total size of log entries between index1 and index2 (inclusive).
// Thread-safe.
// @param index1 Start index, must be >= first_log_index
// @param index2 End index, must be <= last_log_index
// @return Total size in bytes if successful, -1 on error (e.g. invalid indices)
int get_log_size_diff_by_index(int index1, int index2);✏️ Learnings added
Learnt from: AlexStocks
PR: arana-db/braft#10
File: src/braft/node.h:266-267
Timestamp: 2025-02-05T07:08:44.949Z
Learning: The method `get_log_size_diff_by_index` in NodeImpl class calculates total size of log entries between two indices (inclusive). It is thread-safe and returns -1 on error.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+820
to
+821
| int get_log_size_diff_by_index(int index1, int index2); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive API documentation.
As this is a public API, add detailed documentation including:
- Method purpose and use cases
- Parameter requirements and valid ranges
- Return value semantics and error codes
- Thread safety guarantees
- Performance implications
Example:
// Calculate the total size of log entries between two indices.
// This method is useful for:
// - Estimating storage requirements
// - Managing log compaction
// - Monitoring log growth
//
// Thread-safe.
//
// @param index1 Start index of the range (inclusive)
// @param index2 End index of the range (inclusive)
//
// @return
// >0: Total size in bytes of all log entries in the range
// -1: If indices are invalid or out of range
// -2: If index2 < index1
//
// @note Both indices must be within [first_log_index, last_log_index].
// Large ranges may impact performance.
AlexStocks
reviewed
Feb 5, 2025
src/braft/node.cpp
Outdated
| const int64_t last_log_index = _log_manager->last_log_index(); | ||
|
|
||
| if (index1 < first_log_index) { | ||
| LOG(ERROR) << "node " << _group_id << ":" << _server_id |
There was a problem hiding this comment.
这行的效果，冒号前面有一个空格，后面不需要空格么？
AlexStocks
reviewed
Feb 5, 2025
src/braft/node.cpp
Outdated
| return -1; | ||
| } | ||
| if (index2 > last_log_index) { | ||
| LOG(ERROR) << "node " << _group_id << ":" << _server_id |
There was a problem hiding this comment.
这行的效果，冒号前面有一个空格，后面不需要空格么？
panlei-coder
reviewed
Feb 5, 2025
src/braft/node.cpp
Outdated
| @@ -2252,6 +2252,44 @@ void NodeImpl::on_learner_config_apply(LogEntry *entry) { | |||
| } | |||
| } | |||
|
|
|||
| int NodeImpl::get_log_size_diff_by_index(int index1, int index2) { | |||
There was a problem hiding this comment.
感觉可以考虑再增加一个参数 step，因为计算两个 index 之间的日志量大小只需要一个粗略的估计，不需要很精确的计算，直接每 step 间隔用最前面的日志大小来代替这一段的平均大小，这样可以得到一个估计值。
There was a problem hiding this comment.
这样改动量比较大 暂时先以这一版合入吧 我后续跟进一下改成这个按step间日志估算的feature
longfar-ncy
reviewed
Feb 6, 2025
src/braft/node.cpp
Outdated
| @@ -2252,6 +2252,44 @@ void NodeImpl::on_learner_config_apply(LogEntry *entry) { | |||
| } | |||
| } | |||
|
|
|||
| int NodeImpl::get_log_size_diff_by_index(int index1, int index2) { | |||
There was a problem hiding this comment.
我看braft里的index都用int64t表示，这里要不统一一下
There was a problem hiding this comment.
我看braft里的index都用int64t表示，这里要不统一一下
看了下实现比较简单，其它地方问题不大，远哥的这个点比较关键，函数声明啥的都可以同步改一下。
longfar-ncy
reviewed
Feb 6, 2025
| if (!entry) { | ||
| LOG(ERROR) << "node " << _group_id << ":" << _server_id | ||
| << " failed to get log entry at index=" << i; | ||
| return -1; |
There was a problem hiding this comment.
FATAL等级是不是可以直接异常退出？省个 return -1
There was a problem hiding this comment.
了解, 翻阅了一下braft的源码发现LOG(ERROR)不能直接退出, 所以需要保留 return -1
There was a problem hiding this comment.
这个不是 glog 吗 ？ glog 我记得有 FATAL 等级吗？
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/braft/node.cpp(1 hunks)src/braft/node.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/braft/node.h
🧰 Additional context used
🧠 Learnings (1)
src/braft/node.cpp (1)
Learnt from: AlexStocks
PR: arana-db/braft#10
File: src/braft/node.h:266-267
Timestamp: 2025-02-05T07:08:45.052Z
Learning: The method `get_log_size_diff_by_index` in NodeImpl class calculates total size of log entries between two indices (inclusive). It is thread-safe and returns -1 on error.
src/braft/node.cpp
Outdated
Comment on lines
2255
to
2297
| // Calculate the total size of log entries between index1 and index2 (inclusive). | ||
| // Returns: | ||
| // >0: Total size in bytes | ||
| // -1: If indices are invalid or out of range | ||
| // -2: If index2 < index1 | ||
| // Note: Both indices must be within the valid log range [first_log_index, last_log_index] | ||
| int NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) { | ||
| if (index1 > index2) { | ||
| std::swap(index1, index2); | ||
| } | ||
|
|
||
| const int64_t first_log_index = _log_manager->first_log_index(); | ||
| const int64_t last_log_index = _log_manager->last_log_index(); | ||
|
|
||
| if (index1 < first_log_index) { | ||
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | ||
| << " index1=" << index1 << " is out of range, first_log_index=" | ||
| << first_log_index; | ||
| return -1; | ||
| } | ||
| if (index2 > last_log_index) { | ||
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | ||
| << " index2=" << index2 << " is out of range, last_log_index=" | ||
| << last_log_index; | ||
| return -1; | ||
| } | ||
|
|
||
| int total_size = 0; | ||
| for (int64_t i = index1; i <= index2; ++i) { | ||
| LogEntry* entry = _log_manager->get_entry(i); | ||
| if (!entry) { | ||
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | ||
| << " failed to get log entry at index=" << i; | ||
| return -1; | ||
| } | ||
| LOG(INFO) << "node entry index " << i << " : size " << entry->data.length(); | ||
|
|
||
| total_size += entry->data.length(); | ||
| entry->Release(); | ||
| } | ||
|
|
||
| return total_size; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential memory leak and integer overflow issues.
The implementation has the following issues:
- Memory leak: If
get_entrysucceeds butdata.length()causes integer overflow,entry->Release()is not called. - Integer overflow: No protection against overflow when accumulating total_size.
- Logging: Error logs could be more descriptive.
Apply this diff to fix the issues:
int NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) {
if (index1 > index2) {
std::swap(index1, index2);
}
const int64_t first_log_index = _log_manager->first_log_index();
const int64_t last_log_index = _log_manager->last_log_index();
if (index1 < first_log_index) {
LOG(ERROR) << "node " << _group_id << ":" << _server_id
<< " index1=" << index1 << " is out of range, first_log_index="
<< first_log_index;
return -1;
}
if (index2 > last_log_index) {
LOG(ERROR) << "node " << _group_id << ":" << _server_id
<< " index2=" << index2 << " is out of range, last_log_index="
<< last_log_index;
return -1;
}
int64_t total_size = 0;
for (int64_t i = index1; i <= index2; ++i) {
LogEntry* entry = _log_manager->get_entry(i);
if (!entry) {
LOG(ERROR) << "node " << _group_id << ":" << _server_id
<< " failed to get log entry at index=" << i;
return -1;
}
+ const int64_t entry_size = entry->data.length();
+ if (entry_size > std::numeric_limits<int>::max() - total_size) {
+ LOG(ERROR) << "node " << _group_id << ":" << _server_id
+ << " integer overflow when calculating total size at index=" << i
+ << ", current total_size=" << total_size
+ << ", entry_size=" << entry_size;
+ entry->Release();
+ return -1;
+ }
- LOG(INFO) << "node entry index " << i << " : size " << entry->data.length();
+ LOG(INFO) << "node " << _group_id << ":" << _server_id
+ << " entry index=" << i << " size=" << entry_size;
- total_size += entry->data.length();
+ total_size += entry_size;
entry->Release();
}
return total_size;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Calculate the total size of log entries between index1 and index2 (inclusive). | |
| // Returns: | |
| // >0: Total size in bytes | |
| // -1: If indices are invalid or out of range | |
| // -2: If index2 < index1 | |
| // Note: Both indices must be within the valid log range [first_log_index, last_log_index] | |
| int NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) { | |
| if (index1 > index2) { | |
| std::swap(index1, index2); | |
| } | |
| const int64_t first_log_index = _log_manager->first_log_index(); | |
| const int64_t last_log_index = _log_manager->last_log_index(); | |
| if (index1 < first_log_index) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " index1=" << index1 << " is out of range, first_log_index=" | |
| << first_log_index; | |
| return -1; | |
| } | |
| if (index2 > last_log_index) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " index2=" << index2 << " is out of range, last_log_index=" | |
| << last_log_index; | |
| return -1; | |
| } | |
| int total_size = 0; | |
| for (int64_t i = index1; i <= index2; ++i) { | |
| LogEntry* entry = _log_manager->get_entry(i); | |
| if (!entry) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " failed to get log entry at index=" << i; | |
| return -1; | |
| } | |
| LOG(INFO) << "node entry index " << i << " : size " << entry->data.length(); | |
| total_size += entry->data.length(); | |
| entry->Release(); | |
| } | |
| return total_size; | |
| } | |
| // Calculate the total size of log entries between index1 and index2 (inclusive). | |
| // Returns: | |
| // >0: Total size in bytes | |
| // -1: If indices are invalid or out of range | |
| // -2: If index2 < index1 | |
| // Note: Both indices must be within the valid log range [first_log_index, last_log_index] | |
| int NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) { | |
| if (index1 > index2) { | |
| std::swap(index1, index2); | |
| } | |
| const int64_t first_log_index = _log_manager->first_log_index(); | |
| const int64_t last_log_index = _log_manager->last_log_index(); | |
| if (index1 < first_log_index) { | |
| LOG(ERROR) << "node " << _group_id << ":" << _server_id | |
| << " index1=" << index1 << " is out of range, first_log_index=" | |
| << first_log_index; | |
| return -1; | |
| } | |
| if (index2 > last_log_index) { | |
| LOG(ERROR) << "node " << _group_id << ":" << _server_id | |
| << " index2=" << index2 << " is out of range, last_log_index=" | |
| << last_log_index; | |
| return -1; | |
| } | |
| int64_t total_size = 0; | |
| for (int64_t i = index1; i <= index2; ++i) { | |
| LogEntry* entry = _log_manager->get_entry(i); | |
| if (!entry) { | |
| LOG(ERROR) << "node " << _group_id << ":" << _server_id | |
| << " failed to get log entry at index=" << i; | |
| return -1; | |
| } | |
| const int64_t entry_size = entry->data.length(); | |
| if (entry_size > std::numeric_limits<int>::max() - total_size) { | |
| LOG(ERROR) << "node " << _group_id << ":" << _server_id | |
| << " integer overflow when calculating total size at index=" << i | |
| << ", current total_size=" << total_size | |
| << ", entry_size=" << entry_size; | |
| entry->Release(); | |
| return -1; | |
| } | |
| LOG(INFO) << "node " << _group_id << ":" << _server_id | |
| << " entry index=" << i << " size=" << entry_size; | |
| total_size += entry_size; | |
| entry->Release(); | |
| } | |
| return total_size; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/braft/node.cpp (1)
2269-2271: Improve logging consistency and verbosity.
- The log format should be consistent with other logs in the file (using ":" between group_id and server_id).
- Logging entry sizes at INFO level for each entry could be noisy and impact performance.
Apply this diff to improve the logging:
- LOG(ERROR) << "node " << _group_id << " : " << _server_id + LOG(ERROR) << "node " << _group_id << ":" << _server_id << " index1=" << index1 << " is out of range, first_log_index=" << first_log_index; - LOG(ERROR) << "node " << _group_id << " : " << _server_id + LOG(ERROR) << "node " << _group_id << ":" << _server_id << " index2=" << index2 << " is out of range, last_log_index=" << last_log_index; - LOG(ERROR) << "node " << _group_id << " : " << _server_id + LOG(ERROR) << "node " << _group_id << ":" << _server_id << " failed to get log entry at index=" << i; - LOG(INFO) << "node entry index " << i << " : size " << entry->data.length(); + BRAFT_VLOG << "node " << _group_id << ":" << _server_id + << " entry index=" << i << " size=" << entry_size;Also applies to: 2275-2277, 2285-2287, 2289-2289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/braft/node.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/braft/node.cpp (1)
Learnt from: AlexStocks
PR: arana-db/braft#10
File: src/braft/node.h:266-267
Timestamp: 2025-02-05T07:08:45.052Z
Learning: The method `get_log_size_diff_by_index` in NodeImpl class calculates total size of log entries between two indices (inclusive). It is thread-safe and returns -1 on error.
Comment on lines
+2255
to
+2296
| // Calculate the total size of log entries between index1 and index2 (inclusive). | ||
| // Returns: | ||
| // >0: Total size in bytes | ||
| // -1: If indices are invalid or out of range | ||
| // Note: Both indices must be within the valid log range [first_log_index, last_log_index] | ||
| int NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) { | ||
| if (index1 > index2) { | ||
| std::swap(index1, index2); | ||
| } | ||
|
|
||
| const int64_t first_log_index = _log_manager->first_log_index(); | ||
| const int64_t last_log_index = _log_manager->last_log_index(); | ||
|
|
||
| if (index1 < first_log_index) { | ||
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | ||
| << " index1=" << index1 << " is out of range, first_log_index=" | ||
| << first_log_index; | ||
| return -1; | ||
| } | ||
| if (index2 > last_log_index) { | ||
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | ||
| << " index2=" << index2 << " is out of range, last_log_index=" | ||
| << last_log_index; | ||
| return -1; | ||
| } | ||
|
|
||
| int total_size = 0; | ||
| for (int64_t i = index1; i <= index2; ++i) { | ||
| LogEntry* entry = _log_manager->get_entry(i); | ||
| if (!entry) { | ||
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | ||
| << " failed to get log entry at index=" << i; | ||
| return -1; | ||
| } | ||
| LOG(INFO) << "node entry index " << i << " : size " << entry->data.length(); | ||
|
|
||
| total_size += entry->data.length(); | ||
| entry->Release(); | ||
| } | ||
|
|
||
| return total_size; | ||
| } |
There was a problem hiding this comment.
Add thread safety and fix integer overflow issues.
The implementation has several issues:
- Thread safety: The method accesses shared resources without proper synchronization.
- Integer overflow: No protection against overflow when accumulating total_size.
- Type consistency: Using
intfor total_size while braft usesint64_tfor sizes.
Apply this diff to fix the issues:
-int NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) {
+int64_t NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) {
if (index1 > index2) {
std::swap(index1, index2);
}
+ std::unique_lock<raft_mutex_t> lck(_mutex);
const int64_t first_log_index = _log_manager->first_log_index();
const int64_t last_log_index = _log_manager->last_log_index();
if (index1 < first_log_index) {
LOG(ERROR) << "node " << _group_id << " : " << _server_id
<< " index1=" << index1 << " is out of range, first_log_index="
<< first_log_index;
return -1;
}
if (index2 > last_log_index) {
LOG(ERROR) << "node " << _group_id << " : " << _server_id
<< " index2=" << index2 << " is out of range, last_log_index="
<< last_log_index;
return -1;
}
- int total_size = 0;
+ int64_t total_size = 0;
for (int64_t i = index1; i <= index2; ++i) {
LogEntry* entry = _log_manager->get_entry(i);
if (!entry) {
LOG(ERROR) << "node " << _group_id << " : " << _server_id
<< " failed to get log entry at index=" << i;
return -1;
}
+ const int64_t entry_size = entry->data.length();
+ if (entry_size > std::numeric_limits<int64_t>::max() - total_size) {
+ LOG(ERROR) << "node " << _group_id << " : " << _server_id
+ << " integer overflow when calculating total size at index=" << i
+ << ", current total_size=" << total_size
+ << ", entry_size=" << entry_size;
+ entry->Release();
+ return -1;
+ }
LOG(INFO) << "node entry index " << i << " : size " << entry->data.length();
- total_size += entry->data.length();
+ total_size += entry_size;
entry->Release();
}
return total_size;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Calculate the total size of log entries between index1 and index2 (inclusive). | |
| // Returns: | |
| // >0: Total size in bytes | |
| // -1: If indices are invalid or out of range | |
| // Note: Both indices must be within the valid log range [first_log_index, last_log_index] | |
| int NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) { | |
| if (index1 > index2) { | |
| std::swap(index1, index2); | |
| } | |
| const int64_t first_log_index = _log_manager->first_log_index(); | |
| const int64_t last_log_index = _log_manager->last_log_index(); | |
| if (index1 < first_log_index) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " index1=" << index1 << " is out of range, first_log_index=" | |
| << first_log_index; | |
| return -1; | |
| } | |
| if (index2 > last_log_index) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " index2=" << index2 << " is out of range, last_log_index=" | |
| << last_log_index; | |
| return -1; | |
| } | |
| int total_size = 0; | |
| for (int64_t i = index1; i <= index2; ++i) { | |
| LogEntry* entry = _log_manager->get_entry(i); | |
| if (!entry) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " failed to get log entry at index=" << i; | |
| return -1; | |
| } | |
| LOG(INFO) << "node entry index " << i << " : size " << entry->data.length(); | |
| total_size += entry->data.length(); | |
| entry->Release(); | |
| } | |
| return total_size; | |
| } | |
| int64_t NodeImpl::get_log_size_diff_by_index(int64_t index1, int64_t index2) { | |
| if (index1 > index2) { | |
| std::swap(index1, index2); | |
| } | |
| std::unique_lock<raft_mutex_t> lck(_mutex); | |
| const int64_t first_log_index = _log_manager->first_log_index(); | |
| const int64_t last_log_index = _log_manager->last_log_index(); | |
| if (index1 < first_log_index) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " index1=" << index1 << " is out of range, first_log_index=" | |
| << first_log_index; | |
| return -1; | |
| } | |
| if (index2 > last_log_index) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " index2=" << index2 << " is out of range, last_log_index=" | |
| << last_log_index; | |
| return -1; | |
| } | |
| int64_t total_size = 0; | |
| for (int64_t i = index1; i <= index2; ++i) { | |
| LogEntry* entry = _log_manager->get_entry(i); | |
| if (!entry) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " failed to get log entry at index=" << i; | |
| return -1; | |
| } | |
| const int64_t entry_size = entry->data.length(); | |
| if (entry_size > std::numeric_limits<int64_t>::max() - total_size) { | |
| LOG(ERROR) << "node " << _group_id << " : " << _server_id | |
| << " integer overflow when calculating total size at index=" << i | |
| << ", current total_size=" << total_size | |
| << ", entry_size=" << entry_size; | |
| entry->Release(); | |
| return -1; | |
| } | |
| LOG(INFO) << "node entry index " << i << " : size " << entry->data.length(); | |
| total_size += entry_size; | |
| entry->Release(); | |
| } | |
| return total_size; | |
| } |
longfar-ncy
approved these changes
Feb 10, 2025
longfar-ncy
reviewed
Feb 10, 2025
| } | ||
| LOG(INFO) << "node entry index " << i << " : size " << entry->data.length(); | ||
|
|
||
| total_size += entry->data.length(); |
There was a problem hiding this comment.
这个 length() 返回类型是什么？麻烦确认一下，total_size 最好与其保持一致
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LogManager::get_entry()interface to access cached log entries, manage memory/disk consistency, and minimize code changes.Node::get_log_size_diff_by_index()interface to calculate the log size difference within a specified index range [index1, index2].Associated kiwi Issue : arana-db/kiwi#70
Summary by CodeRabbit
New Features
Tests
These updates enhance log management and tracking capabilities, providing detailed insights into log entry sizes and positioning.