Skip to content

CDK: Evaluate response.text only in debug mode #16809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Sep 17, 2022
Merged

CDK: Evaluate response.text only in debug mode #16809

merged 9 commits into from
Sep 17, 2022

Conversation

grubberr
Copy link
Contributor

@grubberr grubberr commented Sep 16, 2022 •
edited
Loading

What

Accessing response.text for logging right after self._session.send can be not desired in some cases.

Some connectors use stream=True request mode and assume that nobody accesses the whole response at once (accessing response.text or response.content) because evaluation of response.text can force read all data at once.

I propose to access the whole response at once response.text only in debug mode.

@grubberr
self.logger.isEnabledFor added
5b48584 
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr
bump 0.1.87
b04f3fe 
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr grubberr requested review from a team and removed request for a team September 16, 2022 13:14
@github-actions github-actions bot added the CDK Connector Development Kit label Sep 16, 2022
@grubberr grubberr changed the title CDK: Lazy evaluate response.body for debugging CDK: Evaluate response.text only in debug mode Sep 16, 2022
@grubberr
CHANGELOG.md updated
e134deb 
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr
CHANGELOG.md updated
5f761a6 
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr grubberr self-assigned this Sep 16, 2022
alafanechere
alafanechere approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks you for catching this.
0.1.87 was already released, please bump again.

@grubberr grubberr requested a review from a team September 16, 2022 15:07
@grubberr
Copy link
Contributor Author

grubberr commented Sep 17, 2022 •
edited by github-actions bot
Loading

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3072768704
✅ https://github.com/airbytehq/airbyte/actions/runs/3072768704

@grubberr
Copy link
Contributor Author

grubberr commented Sep 17, 2022 •
edited by github-actions bot
Loading

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3072792152
✅ https://github.com/airbytehq/airbyte/actions/runs/3072792152

@grubberr grubberr merged commit a82f59e into master Sep 17, 2022
@grubberr grubberr deleted the grubberr/airbyte_cdk_fix_send_debug branch September 17, 2022 09:12
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Sep 21, 2022
Merged
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
@grubberr @robbinhan
CDK: Evaluate response.text only in debug mode (airbytehq#16809)
ec54ccf 
Signed-off-by: Sergey Chvalyuk <[email protected]>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@grubberr @jhammarstedt
CDK: Evaluate response.text only in debug mode (airbytehq#16809)
7882efd 
Signed-off-by: Sergey Chvalyuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alafanechere alafanechere alafanechere approved these changes

Assignees

@grubberr grubberr

Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@grubberr @alafanechere