Conversation
WalkthroughExpands and hardens the RocksDB open sequence: caps open files before DB::Open, documents paranoid_checks context, and adds full cleanup (Close, delete/nullify Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant DS as RocksDBCloudDataStore
participant R as RocksDB
Caller->>DS: StartDB() / OpenCloudDB()
DS->>DS: set skip_cloud_files_in_getchildren = true\nset options (unordered_write, max_open_files=128)\n(comment: paranoid_checks=false rationale)
DS->>R: DB::Open(...)
R-->>DS: OpenStatus
alt Open succeeds
DS->>DS: pause background work
alt pause ok
DS->>DS: get current epoch
alt epoch ok
DS->>R: SetOptions(enable auto compactions)
R-->>DS: SetOptionsStatus
alt SetOptions ok
DS->>R: SetDBOptions({"max_open_files","-1"})
alt restore ok
DS->>DS: restore skip_cloud_files_in_getchildren = false
DS-->>Caller: return true
else restore failed
DS->>DS: Close(), delete db_, nullify pointers, clear ttl_compaction_filter_
DS-->>Caller: return false
end
else SetOptions failed
DS->>DS: Close(), delete db_, nullify pointers, clear ttl_compaction_filter_
DS-->>Caller: return false
end
else epoch failed
DS->>DS: Close(), delete db_, nullify pointers, clear ttl_compaction_filter_
DS-->>Caller: return false
end
else pause failed
DS->>DS: Close(), delete db_, nullify pointers, clear ttl_compaction_filter_
DS-->>Caller: return false
end
else Open failed
DS->>DS: Close() if needed, delete db_, nullify pointers, clear ttl_compaction_filter_
DS-->>Caller: return false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
603-610: Consider making the temporary open file limit configurable.The temporary cap of 32 is a reasonable workaround for the slow DB open issue, but the magic number lacks justification. Consider:
- Documenting why 32 was chosen (e.g., empirical testing, typical workload requirements)
- Making this value configurable via a parameter if different deployments have different needs
- Adding a constant like
TEMP_MAX_OPEN_FILES_DURING_INITfor clarityExample refactor:
+ // Temporary limit during initialization to avoid slow Open() and LRU issues + constexpr int TEMP_MAX_OPEN_FILES_DURING_INIT = 32; + // The max_open_files default value is -1, it cause DB open all files on // DB::Open() This behavior causes 2 effects, // 1. DB::Open() will be slow // 2. During DB::Open, some of the opened sst files keep in LRUCache will be // deleted due to LRU policy, which causes DB::Open failed // set max_open_files to 0 will conflict with // skip_cloud_files_in_getchildren - options.max_open_files = 32; + options.max_open_files = TEMP_MAX_OPEN_FILES_DURING_INIT;
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
703-708: Add cleanup before returning on SetOptions failure.Returning false here leaves the database in a partially initialized state (opened with background work resumed but auto compactions disabled), causing resource leaks and state inconsistencies.
Before returning, perform cleanup as suggested in the previous review:
// Enable auto compactions after blocking purger status = db_->SetOptions({{"disable_auto_compactions", "false"}}); if (!status.ok()) { LOG(ERROR) << "Fail to enable auto compactions, error: " << status.ToString(); + // Clean up the partially initialized database + db_->Close(); + delete db_; + db_ = nullptr; + ttl_compaction_filter_ = nullptr; return false; }
710-711: Add error handling for max_open_files restoration.The restoration of
max_open_filesto its default value lacks error handling. IfSetOptionsfails, the database will continue running with the temporary limit of 128 open files, severely degrading performance during normal operations.Add error handling as suggested in the previous review:
status = db_->SetOptions( {{"max_open_files", "-1"}}); // restore max_open_files to default value + + if (!status.ok()) + { + LOG(ERROR) << "Fail to restore max_open_files to default, error: " + << status.ToString(); + // Clean up the partially initialized database + db_->Close(); + delete db_; + db_ = nullptr; + ttl_compaction_filter_ = nullptr; + return false; + }
🧹 Nitpick comments (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
603-611: Consider making the temporary max_open_files limit configurable.The hardcoded value of 128 may not be optimal for all deployment scenarios. For large-scale deployments with ample resources, this could be unnecessarily restrictive, while for resource-constrained environments, it might still be too high.
Consider adding a configuration parameter (e.g.,
temp_max_open_files_during_init_) with a sensible default of 128, allowing operators to tune this based on their specific environment and workload characteristics.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
697-702: Inconsistent cleanup pattern leaves DB in partially initialized state.When
current_epochis empty, the code callsContinueBackgroundWork()and returns false, but it does NOT close the database or perform the cleanup sequence used in other error paths (lines 674-679, 689-695, 716-721, 731-736). This leaves the database in a partially initialized state with background work running.Apply this diff to use consistent cleanup:
if (current_epoch.empty()) { LOG(ERROR) << "Current epoch from db is empty"; - db_->ContinueBackgroundWork(); + // Clean up the partially initialized database + db_->Close(); + delete db_; + db_ = nullptr; + ttl_compaction_filter_ = nullptr; return false; }
🧹 Nitpick comments (2)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
603-611: Consider using a named constant or configuration parameter.The temporary
max_open_files = 128limit is hardcoded. While the rationale is sound and well-documented, consider making this value configurable or defining it as a named constant at the class or file level for easier maintenance and tuning.Example:
// At class level or file scope static constexpr int TEMP_MAX_OPEN_FILES_ON_DB_OPEN = 128; // Then use: options.max_open_files = TEMP_MAX_OPEN_FILES_ON_DB_OPEN;
689-695: Cleanup pattern is correct but duplicated.The cleanup logic is correct and consistent. However, this exact pattern is repeated in multiple locations (lines 674-679, 689-695, 716-721, 731-736). Consider extracting it into a private helper method to reduce duplication and ensure consistency.
Example helper method:
// Add to class definition private: void CleanupPartiallyInitializedDB() { if (db_ != nullptr) { db_->Close(); delete db_; db_ = nullptr; } ttl_compaction_filter_ = nullptr; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp(5 hunks)
🔇 Additional comments (3)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (3)
674-679: LGTM! Proper cleanup on failure.The cleanup sequence correctly prevents the database from being left in a partially initialized state when
PauseBackgroundWorkfails.
712-722: LGTM! Addresses past review comment.The error handling and cleanup for auto compaction failures now properly closes the database and prevents partial initialization. This directly addresses the concern raised in the previous review.
Based on past review comments.
724-737: LGTM! Addresses past review comment and PR objective.The restoration of
max_open_filesto-1with proper error handling ensures that:
- The temporary limit of 128 is not silently left in effect (addresses past review concern)
- The database will use unlimited file handles after successful initialization (addresses PR objective)
- Any failure during restoration is properly cleaned up
Based on past review comments.
91ec2a8 to
dbc5298
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
606-614: Consider making the file limit configurable.The hardcoded value of 128 for
max_open_filesduring DB::Open is reasonable, but lacks justification for why 128 specifically. Consider making this configurable or documenting why 128 is the optimal value (e.g., based on benchmarking, typical workload patterns, or system limits).Example:
+ // Set max_open_files to a smaller value during DB::Open to avoid issues + // Default value from config or 128 if not specified + int max_files_during_open = config.max_files_during_open > 0 + ? config.max_files_during_open : 128; // The max_open_files default value is -1, it cause DB open all files on // DB::Open() This behavior causes 2 effects, // 1. DB::Open() will be slow // 2. During DB::Open, some of the opened sst files keep in LRUCache will be // deleted due to LRU policy, which causes DB::Open failed // set max_open_files to 0 will conflict with // skip_cloud_files_in_getchildren // Given a smaller value here to avoid opening too many files - options.max_open_files = 128; + options.max_open_files = max_files_during_open;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp(4 hunks)
🔇 Additional comments (3)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (3)
677-682: Excellent cleanup implementation on error paths.The consistent cleanup pattern (Close, delete, nullify pointers) across all three error paths prevents partially initialized database state and addresses the concerns raised in previous reviews. The approach ensures:
- Proper shutdown via
Close()before resource deallocation- Memory cleanup via
delete db_- Prevention of use-after-free via pointer nullification
- Consistent error handling across all failure scenarios
Based on past review comments.
Also applies to: 692-697, 718-723
702-703: Pre-existing inconsistent error handling.Lines 702-703 contain a pre-existing error path that handles failures differently from the newly added cleanup patterns. When the current epoch is empty, it calls
ContinueBackgroundWork()but doesn't perform the full cleanup sequence (Close, delete, nullify pointers) that other error paths now implement.This could leave the database in a partially initialized state similar to the issues addressed elsewhere in this PR.
Consider whether this error path should also be updated to follow the consistent cleanup pattern:
if (current_epoch.empty()) { LOG(ERROR) << "Current epoch from db is empty"; - db_->ContinueBackgroundWork(); + // Clean up the partially initialized database + db_->Close(); + delete db_; + db_ = nullptr; + ttl_compaction_filter_ = nullptr; return false; }Note: This is existing code not changed in this PR, but worth reviewing for consistency.
726-739: Proper error handling for max_open_files restoration.The
SetDBOptionscall correctly resetsmax_open_filesto-1(RocksDB’s default), and comprehensive error handling with cleanup on failure is in place.
dbc5298 to
5fae0e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
700-705: Add cleanup before returning when current_epoch is empty.This code path resumes background work but doesn't clean up the database before returning false, leaving it in a partially initialized state. This is inconsistent with the other error paths that were fixed (lines 678-683, 693-698, 715-725).
Apply this diff to add proper cleanup:
if (current_epoch.empty()) { LOG(ERROR) << "Current epoch from db is empty"; - db_->ContinueBackgroundWork(); + // Clean up the partially initialized database + db_->Close(); + delete db_; + db_ = nullptr; + ttl_compaction_filter_ = nullptr; return false; }
🧹 Nitpick comments (2)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
334-334: Remove or uncomment this line for consistency.Line 334 is commented out, which means
skip_cloud_files_in_getchildrenis never set totrue. However, line 669 still sets it back tofalse, making that code redundant. Either:
- Remove this commented line entirely if the optimization is no longer needed, or
- Uncomment it if you still want the optimization during DB::Open
The current state suggests incomplete refactoring.
448-448: Remove commented-out code.The commented-out
paranoid_checks = falseline should be removed rather than left as a comment. Dead code clutters the codebase and can cause confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp(6 hunks)
🔇 Additional comments (3)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (3)
607-615: Verify the max_open_files limit of 128.The approach of temporarily capping
max_open_filesto 128 during DB::Open and restoring it to -1 afterwards is sound and well-documented. The rationale addresses the PR objectives about preventing the database from loading all files.However, verify that 128 is an appropriate limit for your use case. Consider whether this value should be:
- Made configurable via the RocksDB config
- Tuned based on expected workload or available system resources
678-683: LGTM! Cleanup code properly handles failure paths.The cleanup code correctly handles partial initialization failures by:
- Closing the database handle
- Deleting and nullifying the db_ pointer
- Clearing the ttl_compaction_filter_
This ensures the object is not left in a partially initialized state.
Also applies to: 693-698, 715-725
727-740: LGTM! Proper restoration of max_open_files with error handling.The code correctly:
- Uses
SetDBOptionsfor the DB-widemax_open_filesoption- Restores the default value of -1 to allow unlimited file handles during normal operation
- Includes proper error handling and cleanup if restoration fails
This completes the fix for the txlog open abort issue described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
611-619: Consider making the max_open_files cap configurable.The fix properly addresses the PR objective by capping
max_open_filesto 128 duringDB::Open, preventing the issue where -1 causes all files to be loaded. The value is then restored to -1 after opening (lines 731-732).However, the hard-coded value of 128 might not be optimal for all deployments. Consider making this configurable through a constructor parameter or configuration file to allow tuning based on specific deployment characteristics.
682-687: Extract duplicated cleanup logic into a helper method.The same cleanup pattern appears in four locations (lines 682-687, 697-702, 723-728, 738-743). Each performs:
db_->Close(),delete db_,db_ = nullptr, andttl_compaction_filter_ = nullptr.Consider extracting this into a private helper method:
void CleanupPartiallyInitializedDB() { if (db_ != nullptr) { db_->Close(); delete db_; db_ = nullptr; } ttl_compaction_filter_ = nullptr; }Then replace each cleanup block with a single call to
CleanupPartiallyInitializedDB(). This reduces duplication and ensures consistency if the cleanup logic needs to be modified in the future.Also applies to: 697-702, 723-728, 738-743
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp(6 hunks)
🔇 Additional comments (7)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (7)
332-334: LGTM - Enhanced documentation.The expanded comment clearly documents the additional components involved in the DB open speed-up, providing better context for the
skip_cloud_files_in_getchildrenflag.
449-452: Documented tradeoff: paranoid_checks disabled during open.The comment clearly explains that
paranoid_checksis disabled because consistency checks would fail whenskip_cloud_files_in_getchildrenis true. Sinceskip_cloud_files_in_getchildrenis restored tofalseafterDB::Opencompletes (line 673), this tradeoff is limited to the open phase.Based on learnings
668-673: LGTM - Proper restoration of temporary flag.The code correctly restores
skip_cloud_files_in_getchildrentofalseafterDB::Opencompletes, ensuring normal operation. This pairs with the initial setting at line 335.
682-687: Proper cleanup on PauseBackgroundWork failure.The cleanup code correctly closes the database and releases resources before returning, preventing partially initialized state.
Based on learnings
697-702: Proper cleanup on GetCurrentEpoch failure.The cleanup code correctly closes the database and releases resources before returning, preventing partially initialized state.
Based on learnings
719-729: Proper cleanup on SetOptions failure.The cleanup code correctly closes the database and releases resources before returning, preventing partially initialized state.
Based on learnings
731-744: LGTM - Proper restoration of max_open_files with error handling.The code correctly restores
max_open_filesto its default value (-1) after successfully opening the database with the temporary limit of 128. The error handling properly cleans up resources if restoration fails, addressing previous review feedback.This completes the fix for the PR objective: the database opens with a limited number of files (128) to avoid loading all files, then switches to unlimited files for normal operation.
Based on learnings
max_open_files could not be -1, otherwise DB will load all files
Summary by CodeRabbit
Performance
Bug Fixes
Reliability