feat(clp-s): Add support for ingestion from S3 in the kv-ir ingestion flow; Improve error messages in the kv-ir ingestion flow. #706
Conversation
… for kv-ir ingestion flow
WalkthroughThe pull request introduces a refactoring of error handling in the Changes
Sequence DiagramsequenceDiagram
participant Parser as JsonParser
participant Reader as NetworkReader
participant Logger as ErrorLogger
Parser->>Reader: check_and_log_curl_error()
Reader-->>Parser: CURL return code
alt CURL error detected
Parser->>Logger: Log error details
Parser-->>Parser: Return false
else No error
Parser-->>Parser: Return true
end
✨ 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: 0
🧹 Nitpick comments (1)
components/core/src/clp_s/JsonParser.cpp (1)
904-907: Fix typo in error message.There's a typo in the error message: "unkown" should be "unknown".
- "Encountered unkown IR unit type ({}) during deserialization.", + "Encountered unknown IR unit type ({}) during deserialization.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp_s/JsonParser.cpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
components/core/src/clp_s/JsonParser.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build (macos-latest)
🔇 Additional comments (6)
components/core/src/clp_s/JsonParser.cpp (6)
808-808: LGTM! Good choice of buffer size.The 64KB buffer size is a reasonable default that balances memory usage with performance.
810-815: LGTM! Proper error handling for reader creation.The code correctly handles the case where reader creation fails and ensures proper cleanup.
824-831: LGTM! Comprehensive error handling for deserializer creation.Good error handling with detailed logging that includes both error code and message. The code also ensures proper cleanup of resources.
850-858: LGTM! Proper error handling for deserialization failures.The code properly handles deserialization errors with detailed logging and cleanup.
931-952: LGTM! Well-structured CURL error handling.The new method
check_and_log_curl_erroreffectively centralizes CURL error handling logic. It:
- Safely checks if the reader is a NetworkReader
- Properly extracts and logs both error code and message
- Handles the case where error message might be unavailable
815-819: Consider adding error handling for decompressor.open.The decompressor.open call could potentially fail, but there's no error handling for this case.
Run this script to check if decompressor.open can fail:
Description
This PR adds support for ingesting kv-ir data from S3 by using the same ReaderUtils::try_create_reader flow used by most of the rest of clp-s.
While touching this code we also add extra log messages for errors encountered while deserializing kv-ir.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Refactor
Performance
These changes enhance the robustness and maintainability of the JSON parsing process by centralizing error detection and logging.