remote redis dependency#2109
Conversation
jinke446
requested review from
alogfans,
chestnut-Q and
doujiang24
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of Redis configuration by moving the retrieval of credentials and database indices from the transfer engine's initialization logic directly into the MetaStore factory. It removes several overloaded constructors and configuration parameters in favor of environment variables (MC_REDIS_PASSWORD, MC_REDIS_USERNAME, and MC_REDIS_DB_INDEX). Feedback was provided regarding the new environment variable parsing logic in metastore.cpp, specifically pointing out incorrect defaulting behavior for the database index, contradictory error handling where the code returns a null pointer despite logging a fallback to a default value, and general redundancy in the implementation.
| int redis_db_index_config = 0; | ||
| const char* env_db_index = std::getenv("MC_REDIS_DB_INDEX"); | ||
| if (env_db_index && *env_db_index) { | ||
| try { | ||
| redis_db_index_config = std::stoi(env_db_index); | ||
| } catch (const std::exception& e) { | ||
| LOG(WARNING) << "Invalid REDIS_DB_INDEX environment variable: " | ||
| << env_db_index; | ||
| return nullptr; | ||
| } | ||
| } | ||
|
|
||
| // Validate redis_db_index range (0-255) | ||
| uint8_t db_index = REDIS_DEFAULT_DB_INDEX; | ||
| if (redis_db_index_config >= 0 && | ||
| redis_db_index_config <= REDIS_MAX_DB_INDEX) { | ||
| db_index = static_cast<uint8_t>(redis_db_index_config); | ||
| } else { | ||
| LOG(WARNING) << "Invalid Redis DB index: " << redis_db_index_config | ||
| << ", using default " | ||
| << static_cast<int>(REDIS_DEFAULT_DB_INDEX); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
The logic for processing MC_REDIS_DB_INDEX has several issues:
- Incorrect Defaulting:
redis_db_index_configis initialized to0(line 55). If the environment variable is missing, the range check (lines 69-71) will succeed (since0is a valid index), anddb_indexwill be set to0, potentially overwritingREDIS_DEFAULT_DB_INDEXif it is non-zero. - Contradictory Error Handling: The log message in the
elseblock (lines 73-75) claims that the default value will be used, but the function then returnsnullptr(line 76), which will trigger a fatal error later in the execution flow. - Redundancy: The logic can be simplified by directly parsing the environment variable into the target variable with proper validation.
uint8_t db_index = REDIS_DEFAULT_DB_INDEX;
const char* env_db_index = std::getenv("MC_REDIS_DB_INDEX");
if (env_db_index && *env_db_index) {
try {
int val = std::stoi(env_db_index);
if (val >= 0 && val <= REDIS_MAX_DB_INDEX) {
db_index = static_cast<uint8_t>(val);
} else {
LOG(WARNING) << "Redis DB index out of range: " << val;
return nullptr;
}
} catch (const std::exception& e) {
LOG(WARNING) << "Invalid REDIS_DB_INDEX environment variable: " << env_db_index;
return nullptr;
}
}
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Description
Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-pg)mooncake-rl)Type of Change
How Has This Been Tested?
Checklist
./scripts/code_format.shbefore submitting.