fix: (CDK) (AsyncRetriever) - Add the request and response to each async operations#356
Conversation
|
This linters check will FAIL until this PR is merged and the |
📝 WalkthroughWalkthroughThe changes introduce enhancements across three components. In the response extractor, a comment is added in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Repo as AsyncHttpJobRepository
participant HTTP as HTTP Endpoint
participant Logger as Logger
Client ->> Repo: Call _start_job_and_validate_response()
Repo ->> HTTP: send_request(log_formatter)
HTTP -->> Repo: Return HTTP Response
Repo ->> Logger: format_http_message(http message)
Repo -->> Client: Return Response
Suggested labels
Suggested reviewers
Would you like to see any further adjustments or additional diagrams for clarity, wdyt? 📜 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 (8)
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)
airbyte_cdk/sources/declarative/requesters/http_job_repository.py (1)
254-254: Minor typo in variable name.There's a typo in
delete_job_reponse(missing 's'). Wdyt about fixing it?- delete_job_reponse = self.delete_requester.send_request( + delete_job_response = self.delete_requester.send_request(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/extractors/response_to_file_extractor.py(1 hunks)airbyte_cdk/sources/declarative/requesters/http_job_repository.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/sources/declarative/extractors/response_to_file_extractor.py
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/requesters/http_job_repository.py
[error] 76-76: Unexpected keyword argument 'type' for 'format_http_message' [call-arg]
[error] 131-131: Unexpected keyword argument 'type' for 'format_http_message' [call-arg]
[error] 240-240: Unexpected keyword argument 'type' for 'format_http_message' [call-arg]
[error] 256-256: Unexpected keyword argument 'type' for 'format_http_message' [call-arg]
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/requesters/http_job_repository.py (2)
26-26: LGTM! Good addition of http logging.The import of
format_http_messagealigns well with the PR objectives to add request and response logging to async operations.
74-84: Great job on consistent logging across async operations!The logging additions provide excellent context for each async operation with descriptive titles and messages. This will help with debugging and monitoring of async jobs.
Also applies to: 129-139, 238-248, 254-264
🧰 Tools
🪛 GitHub Actions: Linters
[error] 76-76: Unexpected keyword argument 'type' for 'format_http_message' [call-arg]
…riever-operation-auxiliary-requests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/retrievers/async_retriever.py (1)
107-110: Consider enhancing logging for request/response?The slice logging is a good addition, but to align with the PR objectives of adding request/response to async operations, would it make sense to extend the logging to include HTTP request/response details? We could potentially:
- Add request/response fields to the slice log message
- Create separate log messages for request/response events
What approach would you prefer? 🎯
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/retrievers/async_retriever.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/retrievers/async_retriever.py (3)
4-4: Clean addition of logging capability!The slice logger field is well-structured with appropriate default factory and typing. Nice job! 🎯
Also applies to: 18-18, 31-34
81-97: Verify alignment with PR objectives?The method is well-implemented and documented, but I notice it's focused on cursor slice info rather than the PR's objective of adding request/response to async operations. Would it make sense to include request/response info in the cursor slice, or should this be handled separately? wdyt? 🤔
99-100: Nice optimization withyield from!The change to
yield fromimproves memory efficiency by avoiding the creation of intermediate collections. Well done! 🚀
…riever-operation-auxiliary-requests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/retrievers/async_retriever.py (1)
31-34: Consider using a regular instance attribute instead of field?The
slice_loggeris initialized with a default factory but marked asinit=False. Since it's a simple initialization with no dependencies, we could potentially simplify this to a regular instance attribute. wdyt?- slice_logger: AlwaysLogSliceLogger = field( - init=False, - default_factory=lambda: AlwaysLogSliceLogger(), - ) + slice_logger: AlwaysLogSliceLogger = AlwaysLogSliceLogger()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/retrievers/async_retriever.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Analyze (python)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (4)
airbyte_cdk/sources/declarative/retrievers/async_retriever.py (4)
81-97: Well-documented helper method, LGTM! 👍The
_get_cursor_slicemethod is nicely documented and follows a clear single responsibility principle. The implementation is concise and handles edge cases appropriately.
99-100: Good use of yield from! 🎯Nice improvement using
yield frominstead of return for the generator method.
1-121: Missing request/response logging mentioned in PR objectivesThe PR objectives mention adding request and response to async operations, but I don't see those changes in this file. Should we add similar logging for HTTP requests/responses in the async operations (
_get_validated_polling_response(),_start_job_and_validate_response(), etc.)?#!/bin/bash # Search for async operation methods that might need request/response logging ast-grep --pattern $'class AsyncRetriever { $$$ _get_validated_polling_response($_) { $$$ } $$$ }'Likely an incorrect or invalid review comment.
107-110:Details
✅ Verification successful
Verify impact on existing logs
The slice logging addition looks good, but let's ensure it doesn't affect existing log consumers. Would you mind confirming if this new log message could interfere with any existing log parsing or monitoring systems?
🏁 Script executed:
#!/bin/bash # Search for existing log message handling or parsing rg -g '!*.pyc' -g '!*test*' 'slice.*log' .Length of output: 6530
Verified: Existing slice log messages are consistent
I searched the repo for "slice.*log" and confirmed that the usage of
create_slice_log_messagein the async retriever mirrors the pattern found in other parts of the codebase (e.g., in concurrent and streams modules). It appears that the new log message maintains compatibility with existing log consumers and should not interfere with current log parsing or monitoring systems. wdyt?
Aldo Gonzalez (aldogonzalez8)
left a comment
Aldo Gonzalez (aldogonzalez8)
left a comment
There was a problem hiding this comment.
APPROVED
Baz (bazarnov)
deleted the
baz/cdk/add-async-retriever-operation-auxiliary-requests
branch
3 participants
What
Resolves:
How
format_http_messageto the following methods while handling theresponse:_get_validated_polling_response()- to track theASYNC_POLLmessage types_start_job_and_validate_response()- to track theASYNC_CREATEmessage typesabort()- to track theASYNC_ABORTmessage typesdelete()- to track theASYNC_DELETEmessage typesUser Impact
No impact is expected this is not a breaking change
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation