fix: ensure panic safety #1212
fix: ensure panic safety #1212
Conversation
WalkthroughThis pull request introduces a new constant Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
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: 1
🧹 Nitpick comments (1)
src/parseable/streams.rs (1)
487-488: Improved error messaging but still has panic potentialReplacing
unwrap()withexpect("File was just created")provides better context when an error occurs. However, this still has the potential to panic.Consider using a
matchpattern similar to what you implemented on lines 504-513 to fully eliminate panic potential in all cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/parseable/mod.rs(1 hunks)src/parseable/streams.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/parseable/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (3)
src/parseable/streams.rs (3)
64-64: Good improvement in constant usageUsing the imported constant
ARROW_FILE_EXTENSIONinstead of a local definition enhances code maintainability and consistency across the codebase.
491-492: Improved logging formatThe updated format string makes the log message more concise and easier to read.
519-519: Enhanced consistency using a constantUsing the imported constant
ARROW_FILE_EXTENSIONinstead of a hardcoded string ensures consistency across metrics labels. If the file extension ever changes, it will only need to be updated in one place.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/parseable/staging/reader.rs (1)
85-102: Improved panic safety and error handling.The method has been refactored to handle file operations more safely, preventing potential panics. Key improvements include:
- Better parameter naming (
file_pathsinstead offiles)- Using pattern matching with
let Ok(file) = File::open(&path) else {...}instead of unwrapping- Appropriate logging levels (warning for file open failures, error for invalid files)
- Explicitly skipping problematic files with
continueConsider enhancing the warning message to include the actual error details, not just the path:
- warn!("Error when trying to read file: {path:?}"); + warn!("Error when trying to read file {path:?}: {err}");This would require capturing the error in the pattern matching:
- let Ok(file) = File::open(&path) else { + let Ok(file) = File::open(&path).map_err(|err| { + warn!("Error when trying to read file {path:?}: {err}"); + }) else { - warn!("Error when trying to read file: {path:?}"); continue; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/parseable/staging/reader.rs(2 hunks)
🔇 Additional comments (3)
src/parseable/staging/reader.rs (3)
33-33: Enhanced logging with appropriate severity levels.Adding the
warnimport allows using different logging levels based on error severity, which is a good practice for more granular log analysis.
93-96: Consistent error handling pattern.The error handling pattern applied to file opening is consistently applied to the reverse reader creation as well, maintaining a uniform approach to error handling throughout the method.
87-91:❓ Verification inconclusive
Verify the impact on file processing metrics.
The change from error logging to warning for file open failures might affect how many files are reported as failed in metrics/monitoring. Ensure this doesn't hide important issues that should be tracked.
🏁 Script executed:
#!/bin/bash # Look for any metrics or monitoring code that might be affected by the logging change rg -A 3 -B 3 "error.*trying to read file" --type rust rg -A 3 -B 3 "failed.*files" --type rust rg -A 3 -B 3 "file.*metrics" --type rustLength of output: 697
Action: Verify Monitoring Impact from Logging Level Change
The downgrade from error to warning when a file open fails in
src/parseable/staging/reader.rsmight reduce the signal used by our monitoring to flag failed file reads. While a scan of the codebase (notably insrc/parseable/streams.rs) shows that metrics for staging files are being explicitly checked—e.g., viametrics::STAGING_FILES—it’s unclear whether file open failures in the reader are correlated with these metrics. Please verify that file read errors still contribute to tracking failures in our metrics pipeline. Consider whether an explicit metric counter should be incremented alongside logging rather than relying solely on log severity for monitoring.
Signed-off-by: Devdutt Shenoi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/storage/object_storage.rs (1)
464-470: Improved error handling with cleaner, more idiomatic codeThe refactoring of the
get_manifestmethod simplifies the error handling by using the?operator for deserialization errors instead of a nested match statement. This change aligns perfectly with the PR objective of ensuring panic safety, as it properly propagates any deserialization errors rather than potentially causing a panic.The code is now more concise and follows Rust's idiomatic error handling patterns, making it easier to read and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/storage/object_storage.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
Signed-off-by: Devdutt Shenoi <[email protected]>
Fixes #XXXX.
Description
Don't panic when file metadata can't be extracted because file doesn't exist
This PR has:
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Refactor