Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat:Statistics ticker count #2769

Merged
merged 13 commits into from
Aug 9, 2024

Conversation

baixin01
Copy link
Collaborator

@baixin01 baixin01 commented Jun 28, 2024

related issue #2685

1.配置中增加 open_rocksdb_statistics_tickers ,默认为no。开启会损耗+1.5%
2.info rocksdb统计 statistics统计

Summary by CodeRabbit

  • New Features

    • Introduced new configuration options to enable or disable database statistics and set their detail level.
  • Improvements

    • Enhanced the retrieval of RocksDB information to optionally include detailed statistics based on user-defined settings.
  • Bug Fixes

    • Implemented safeguards to ensure the statistics level cannot be set below zero.

Copy link

coderabbitai bot commented Jun 28, 2024

Walkthrough

These changes enhance the pika.conf configuration by introducing parameters for managing database statistics, specifically enable-db-statistics and db-statistics-level. This allows users to toggle statistics logging and specify the detail level. Various components have been updated to ensure that RocksDB statistics can be easily managed, improving overall functionality and user control.

Changes

Files Change Summary
conf/pika.conf Introduced enable-db-statistics, db-statistics-level, and modified open_rocksdb_statistics_tickers.
include/pika_conf.h Added methods enable_db_statistics() and db_statistics_level() with corresponding member variables.
src/pika_admin.cc, src/storage/src/redis.cc, src/storage/src/storage.cc Updated GetRocksDBInfo method to include a new parameter for conditional statistics logging.
src/pika_conf.cc Enhanced Load and ConfigRewrite methods to manage new statistics configurations.
src/storage/include/storage/storage.h Updated GetRocksDBInfo signature to accept the new open_ticker parameter.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Pika
    participant Config
    participant RocksDB
    participant Redis

    User->>+Pika: Update pika.conf (enable-db-statistics)
    Pika->>+Config: Load configuration
    Config->>Config: Set enable_db_statistics_
    Config-->>-Pika: Configuration loaded

    User->>+Pika: Request RocksDB Info
    Pika->>+Config: Check enable_db_statistics()
    Config-->>-Pika: Return boolean value
    Pika->>+Redis: Call GetRocksDBInfo with boolean parameter
    Redis->>RocksDB: Fetch info with or without ticker based on parameter
    RocksDB-->>Redis: Return info details
    Redis-->>Pika: Return formatted info
    Pika-->>User: Display RocksDB Info
Loading

Poem

In the forest of code, new choices bloom,
Statistics dancing, dispelling the gloom.
Tickers can flutter, or rest for a while,
With Pika's new options, we manage in style.
Configuration’s magic, now clearer than night,
Enhancing our tools, making features shine bright! 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the ✏️ Feature New feature or request label Jun 28, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1be0738 and 71f6186.

Files selected for processing (7)
  • conf/pika.conf (1 hunks)
  • include/pika_conf.h (2 hunks)
  • src/pika_admin.cc (1 hunks)
  • src/storage/include/storage/storage.h (1 hunks)
  • src/storage/src/redis.cc (2 hunks)
  • src/storage/src/redis.h (2 hunks)
  • src/storage/src/storage.cc (1 hunks)
Additional comments not posted (9)
src/storage/src/redis.h (2)

247-247: Added method for RocksDB information retrieval with a new parameter.

The method GetRocksDBInfo has been appropriately updated to include the new bool open_ticker parameter, which aligns with the PR's objective to conditionally provide detailed statistics based on configuration. This change should be cross-verified in the implementation to ensure it handles the parameter correctly.


473-473: Initialization of new member variable.

The member variable db_statistics_ has been initialized to nullptr. This is a standard practice for pointer safety. However, ensure that this variable is properly managed throughout its lifecycle to avoid memory leaks or dangling pointers, especially in the destructor or when copying instances of this class.

src/storage/src/redis.cc (1)

269-269: Comprehensive update to GetRocksDBInfo to include detailed statistics.

The method has been extensively updated to gather and format a wide range of database statistics. The use of lambdas for property and ticker data retrieval is efficient and makes the code cleaner. However, ensure that all used properties and tickers are supported by your version of RocksDB to avoid runtime errors. Additionally, the conditional checks for open_ticker are crucial and must be thoroughly tested to ensure that enabling/disabling statistics works as expected.

Also applies to: 269-458

conf/pika.conf (1)

353-353: Configuration setting added correctly for RocksDB statistics tickers.

The new configuration option open_rocksdb_statistics_tickers is added with a default value of "no", which aligns with the PR's objectives. This option is well-documented and follows the conventions used throughout the configuration file.

include/pika_conf.h (2)

177-179: New method open_rocksdb_statistics_tickers() added correctly.

The method open_rocksdb_statistics_tickers() has been added to allow access to the new configuration setting. It is implemented correctly, using thread-safe access patterns with std::shared_lock, consistent with other getters in the class.


893-893: New member variable open_rocksdb_statistics_tickers_ initialized correctly.

The boolean member variable open_rocksdb_statistics_tickers_ is initialized to false, aligning with the default configuration specified in pika.conf. This is a good practice as it ensures the default behavior is consistent across the application.

src/storage/include/storage/storage.h (1)

1094-1094: Updated method signature to include open_ticker parameter.

The method GetRocksDBInfo now accepts a boolean parameter to conditionally fetch RocksDB statistics. This is a critical change that affects all parts of the system where this method is called.

Verification successful

Updated method signature to include open_ticker parameter.

The method GetRocksDBInfo now accepts a boolean parameter to conditionally fetch RocksDB statistics. This is a critical change that affects all parts of the system where this method is called. The following instances have been verified to include the new parameter:

  • src/pika_admin.cc
    • Line: db_item.second->storage()->GetRocksDBInfo(rocksdb_info, true);
    • Line: db_item.second->storage()->GetRocksDBInfo(rocksdb_info, false);
  • src/storage/src/storage.cc
    • Line: inst->GetRocksDBInfo(info, temp, open_ticker);
  • src/storage/src/redis.cc
    • Line: void Redis::GetRocksDBInfo(std::string& info, const char* prefix, bool open_ticker)

These updates confirm that the method calls have been correctly modified to include the new open_ticker parameter.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify all function calls to `GetRocksDBInfo` include the new boolean parameter.

# Test: Search for the function usage. Expect: Only occurances of the new signature with the boolean parameter.
rg --type cpp 'GetRocksDBInfo'

Length of output: 691

src/storage/src/storage.cc (1)

1924-1928: Review of Storage::GetRocksDBInfo function changes:

The addition of the open_ticker parameter is well-handled in the implementation. The function effectively delegates the responsibility of gathering statistics to the inst->GetRocksDBInfo method of individual instances, which is a good design choice as it maintains separation of concerns.

Ensure that all instances of Redis class (or similar) that implement the GetRocksDBInfo method are updated to handle this new parameter appropriately to avoid runtime errors.

src/pika_admin.cc (1)

1309-1313: Conditional RocksDB statistics collection based on configuration

The implementation correctly toggles the collection of RocksDB statistics based on the open_rocksdb_statistics_tickers configuration setting. This change allows for performance optimizations by disabling detailed statistics when not needed.

Ensure that the GetRocksDBInfo method appropriately handles the open_ticker parameter in all scenarios to prevent any potential issues when statistics collection is toggled.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 71f6186 and 39ab078.

Files selected for processing (2)
  • conf/pika.conf (1 hunks)
  • src/storage/src/redis.cc (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • conf/pika.conf
  • src/storage/src/redis.cc

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 39ab078 and ccf6b41.

Files selected for processing (3)
  • conf/pika.conf (1 hunks)
  • src/pika_admin.cc (2 hunks)
  • src/pika_conf.cc (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • conf/pika.conf
  • src/pika_admin.cc
Additional comments not posted (2)
src/pika_conf.cc (2)

697-701: Proper handling of the open-rocksdb-statistics-tickers configuration.

The code correctly checks the configuration string and sets the boolean value based on it. The default behavior of treating any value other than "yes" as "no" is a safe approach to configuration flags.


805-805: Correct implementation of configuration rewriting for open-rocksdb-statistics-tickers.

The method effectively ensures that the current state of the open_rocksdb_statistics_tickers_ is accurately represented in the configuration file, using a clear and concise approach.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ccf6b41 and 426cb0d.

Files selected for processing (2)
  • include/pika_conf.h (3 hunks)
  • src/pika_admin.cc (4 hunks)
Files skipped from review as they are similar to previous changes (2)
  • include/pika_conf.h
  • src/pika_admin.cc

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 426cb0d and 5c80dbf.

Files selected for processing (2)
  • include/pika_conf.h (3 hunks)
  • src/pika_admin.cc (4 hunks)
Files skipped from review as they are similar to previous changes (2)
  • include/pika_conf.h
  • src/pika_admin.cc

@baixin01
Copy link
Collaborator Author

baixin01 commented Jul 5, 2024

@coderabbitai review
@CodiumAI-Agent /review
@CodiumAI-Agent /improve

Copy link

coderabbitai bot commented Jul 5, 2024

@baixin01: I will review the changes and provide feedback.

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@baixin01
Copy link
Collaborator Author

baixin01 commented Jul 5, 2024

@coderabbitai review

Copy link

coderabbitai bot commented Jul 5, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@baixin01
Copy link
Collaborator Author

baixin01 commented Jul 5, 2024

@CodiumAI-Agent /review

@baixin01
Copy link
Collaborator Author

baixin01 commented Jul 5, 2024

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Configuration Consistency:
Ensure that the configuration option "open-rocksdb-statistics-tickers" is consistently handled across all relevant files and methods. It appears to be well integrated, but thorough testing is needed to confirm that it behaves as expected under different scenarios.

Performance Impact:
The addition of detailed ticker statistics can have a performance impact, as noted in the PR description. It's important to ensure that this feature can be toggled without side effects and does not degrade performance when disabled.

Error Handling:
The error message for invalid argument in "ConfigSet" method should be tested to ensure it provides clear feedback to the user.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5c80dbf and 29c6be5.

Files selected for processing (3)
  • include/pika_conf.h (2 hunks)
  • src/pika_admin.cc (2 hunks)
  • src/storage/src/redis.cc (3 hunks)
Files skipped from review as they are similar to previous changes (3)
  • include/pika_conf.h
  • src/pika_admin.cc
  • src/storage/src/redis.cc

@AlexStocks AlexStocks requested a review from wangshao1 July 5, 2024 13:46
@@ -470,6 +470,7 @@ class Redis {
Storage* const storage_;
std::shared_ptr<LockMgr> lock_mgr_;
rocksdb::DB* db_ = nullptr;
std::shared_ptr<rocksdb::Statistics> db_statistics_ = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

没找到初始化成员变量初始化的地方。

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

代码题错了 done

@@ -887,6 +890,7 @@ class PikaConf : public pstd::BaseConf {
int64_t thread_migrate_keys_num_ = 0;
int64_t max_write_buffer_size_ = 0;
int64_t max_total_wal_size_ = 0;
bool open_rocksdb_statistics_tickers_ = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个命令最好是支持可以动态修改不用重启进程,线上用来定位问题的时候更方便。

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rocksdb statics统计 不支持动态

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 29c6be5 and 7260661.

Files selected for processing (7)
  • conf/pika.conf (1 hunks)
  • include/pika_conf.h (2 hunks)
  • include/pika_repl_client.h (1 hunks)
  • src/pika_admin.cc (1 hunks)
  • src/pika_conf.cc (2 hunks)
  • src/storage/src/redis.cc (4 hunks)
  • src/storage/src/redis.h (1 hunks)
Additional comments not posted (18)
src/storage/src/redis.h (1)

473-473: LGTM! Ensure consistent initialization and usage.

The new member variable db_statistics_ enhances the class's capability to manage RocksDB statistics. Verify that its initialization and usage are consistent across the codebase.

src/storage/src/redis.cc (6)

70-74: LGTM! Ensure consistent initialization and usage.

The new configuration option for enabling database statistics is correctly integrated. Verify that the initialization and usage of db_statistics_ are consistent across the codebase.


278-282: LGTM! The new lambda function improves code clarity.

The write_aggregated_int_property lambda function standardizes the method of reporting integer properties from RocksDB, improving code clarity and consistency.


284-296: LGTM! The new lambda function enhances property reporting.

The write_property lambda function allows for more flexible reporting, accommodating cases where the handles_ vector may be empty or populated. This enhances the robustness of the code when retrieving properties from different column families.


298-303: LGTM! The new lambda function for ticker count reporting is a valuable addition.

The write_ticker_count lambda function tracks various statistics related to database operations, significantly broadening the scope of metrics being reported.


315-372: LGTM! The refactored metrics reporting improves consistency.

The refactored metrics reporting using the new lambda functions ensures consistency in how metrics are handled across the codebase.


375-465: LGTM! The new metrics for blob files enhance monitoring capabilities.

The new metrics for blob files improve the monitoring capabilities of the Redis storage implementation.

conf/pika.conf (2)

349-350: Configuration Addition: enable-db-statistics

The new configuration option enable-db-statistics is correctly added with a default value of no. This allows users to enable or disable database statistics.


351-353: Configuration Addition: db-statistics-level

The new configuration option db-statistics-level is correctly added with a default value of 2. This allows users to set the level of detail for database statistics.

src/pika_conf.cc (3)

697-701: Configuration Loading: open-rocksdb-statistics-tickers

The Load method correctly retrieves the configuration value for open-rocksdb-statistics-tickers and sets the enable_db_statistics_ flag.


702-706: Configuration Loading: db-statistics-level

The Load method correctly retrieves the configuration value for db-statistics-level and sets the db_statistics_level_ variable, ensuring it is not set below zero.


811-812: Configuration Saving: enable-db-statistics and db-statistics-level

The ConfigRewrite method correctly saves the new configuration parameters enable-db-statistics and db-statistics-level.

include/pika_conf.h (4)

177-179: LGTM!

The method enable_db_statistics correctly returns the value of enable_db_statistics_.


180-183: LGTM!

The method db_statistics_level correctly returns the value of db_statistics_level_ with appropriate thread safety.


897-897: LGTM!

The private member variable enable_db_statistics_ is correctly defined and initialized to false.


898-898: LGTM!

The private member variable db_statistics_level_ is correctly defined and initialized to 0.

src/pika_admin.cc (2)

2150-2154: Ensure Consistent Naming for Configuration Parameters

The configuration parameter enable-db-statistics is checked and added correctly. Verify that this parameter is consistently named and used across the entire codebase.


2156-2160: Ensure Consistent Naming for Configuration Parameters

The configuration parameter db-statistics-level is checked and added correctly. Verify that this parameter is consistently named and used across the entire codebase.

@@ -21,7 +21,7 @@
#include "include/pika_repl_client_thread.h"

#include "net/include/thread_pool.h"
#include "pika_inner_message.pb.h"
#include "pika_inner_message.pb. h"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the include directive.

The include directive has an unintended space before the file extension, which could cause issues with file inclusion.

- #include "pika_inner_message.pb. h"
+ #include "pika_inner_message.pb.h"
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
#include "pika_inner_message.pb. h"
#include "pika_inner_message.pb.h"

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7260661 and b9fc80a.

Files selected for processing (4)
  • src/pika_conf.cc (2 hunks)
  • src/pika_server.cc (1 hunks)
  • src/storage/include/storage/storage.h (1 hunks)
  • src/storage/src/redis.cc (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • src/pika_conf.cc
Additional comments not posted (4)
src/storage/src/redis.cc (2)

314-317: LGTM!

The usage of the write_aggregated_int_property and write_property lambda functions for reporting various RocksDB properties is consistent and clear. The refactoring improves readability and maintainability.

Also applies to: 320-321, 324-324, 327-329, 332-332, 335-335, 338-338, 341-342, 345-345, 348-354, 355-361, 362-363, 366-366, 369-371, 374-377, 379-381


383-465: LGTM!

The addition of the write_ticker_count lambda function for reporting various RocksDB ticker counts is well-structured and enhances the monitoring capabilities. The usage is consistent and clear.

src/storage/include/storage/storage.h (1)

72-73: LGTM!

The new members db_statistics_level and enable_db_statistics are correctly added and initialized in the StorageOptions struct. This enhances the configuration capabilities as intended.

src/pika_server.cc (1)

1419-1421: Ensure consistency with other configuration options.

The changes to enable and set the level of database statistics are correctly implemented. However, ensure that these options are consistently applied across the codebase and do not conflict with other configuration settings.

Verification successful

Changes are consistent and correctly implemented.

The configuration options enable_db_statistics and db_statistics_level are consistently applied across the codebase without any conflicts or inconsistencies.

  • src/pika_conf.cc
  • src/pika_admin.cc
  • src/pika_server.cc
  • src/storage/include/storage/storage.h
  • src/storage/src/redis.cc
  • include/pika_conf.h
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistency of the `enable_db_statistics` and `db_statistics_level` options across the codebase.

# Test: Search for the usage of these options. Expect: Consistent usage across the codebase.
rg --type cpp -A 5 $'enable_db_statistics'
rg --type cpp -A 5 $'db_statistics_level'

Length of output: 6377

Comment on lines +277 to +281
auto write_aggregated_int_property=[&](const Slice& property, const char *metric) {
uint64_t value = 0;
db_->GetAggregatedIntProperty(property, &value);
string_stream << prefix << metric << ':' << value << "\r\n";
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for GetAggregatedIntProperty.

The lambda function write_aggregated_int_property does not handle potential errors from db_->GetAggregatedIntProperty. Consider adding error handling to ensure robustness.

-  db_->GetAggregatedIntProperty(property, &value);
+  if (!db_->GetAggregatedIntProperty(property, &value)) {
+    // Handle error appropriately, e.g., log the error or set a default value
+  }
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
auto write_aggregated_int_property=[&](const Slice& property, const char *metric) {
uint64_t value = 0;
db_->GetAggregatedIntProperty(property, &value);
string_stream << prefix << metric << ':' << value << "\r\n";
};
auto write_aggregated_int_property=[&](const Slice& property, const char *metric) {
uint64_t value = 0;
if (!db_->GetAggregatedIntProperty(property, &value)) {
// Handle error appropriately, e.g., log the error or set a default value
}
string_stream << prefix << metric << ':' << value << "\r\n";
};

Comment on lines +297 to +302
auto write_ticker_count = [&](uint32_t tick_type, const char *metric) {
if (db_statistics_ == nullptr) {
return;
}
uint64_t count = db_statistics_->getTickerCount(tick_type);
string_stream << prefix << metric << ':' << count << "\r\n";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for getTickerCount.

The lambda function write_ticker_count does not handle potential errors from db_statistics_->getTickerCount. Consider adding error handling to ensure robustness.

-  uint64_t count = db_statistics_->getTickerCount(tick_type);
+  uint64_t count = 0;
+  if (db_statistics_) {
+    count = db_statistics_->getTickerCount(tick_type);
+  } else {
+    // Handle error appropriately, e.g., log the error or set a default value
+  }
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
auto write_ticker_count = [&](uint32_t tick_type, const char *metric) {
if (db_statistics_ == nullptr) {
return;
}
uint64_t count = db_statistics_->getTickerCount(tick_type);
string_stream << prefix << metric << ':' << count << "\r\n";
auto write_ticker_count = [&](uint32_t tick_type, const char *metric) {
if (db_statistics_ == nullptr) {
return;
}
uint64_t count = 0;
if (db_statistics_) {
count = db_statistics_->getTickerCount(tick_type);
} else {
// Handle error appropriately, e.g., log the error or set a default value
}
string_stream << prefix << metric << ':' << count << "\r\n";

Comment on lines +283 to +295
auto write_property=[&](const Slice& property, const char *metric) {
if (handles_.size() == 0) {
std::string value;
db_->GetProperty(db_->DefaultColumnFamily(), property, &value);
string_stream << prefix << metric << "_" << db_->DefaultColumnFamily()->GetName() << ':' << value << "\r\n";
} else {
for (auto handle : handles_) {
std::string value;
db_->GetProperty(handle, property, &value);
string_stream << prefix << metric << "_" << handle->GetName() << ':' << value << "\r\n";
}
}
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for GetProperty.

The lambda function write_property does not handle potential errors from db_->GetProperty. Consider adding error handling to ensure robustness.

-  db_->GetProperty(handle, property, &value);
+  if (!db_->GetProperty(handle, property, &value).ok()) {
+    // Handle error appropriately, e.g., log the error or set a default value
+  }
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
auto write_property=[&](const Slice& property, const char *metric) {
if (handles_.size() == 0) {
std::string value;
db_->GetProperty(db_->DefaultColumnFamily(), property, &value);
string_stream << prefix << metric << "_" << db_->DefaultColumnFamily()->GetName() << ':' << value << "\r\n";
} else {
for (auto handle : handles_) {
std::string value;
db_->GetProperty(handle, property, &value);
string_stream << prefix << metric << "_" << handle->GetName() << ':' << value << "\r\n";
}
}
};
auto write_property=[&](const Slice& property, const char *metric) {
if (handles_.size() == 0) {
std::string value;
if (!db_->GetProperty(db_->DefaultColumnFamily(), property, &value).ok()) {
// Handle error appropriately, e.g., log the error or set a default value
} else {
string_stream << prefix << metric << "_" << db_->DefaultColumnFamily()->GetName() << ':' << value << "\r\n";
}
} else {
for (auto handle : handles_) {
std::string value;
if (!db_->GetProperty(handle, property, &value).ok()) {
// Handle error appropriately, e.g., log the error or set a default value
} else {
string_stream << prefix << metric << "_" << handle->GetName() << ':' << value << "\r\n";
}
}
}
};

@luky116 luky116 merged commit b5c6843 into OpenAtomFoundation:unstable Aug 9, 2024
12 checks passed
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0.1 ✏️ Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants