Skip to content
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.

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

Feature/optional last response preserving #146

Merged

Conversation

rosh-n-ix
Copy link

Issue:
When fetching clinical data, there are cases where files exceeding 2 GB are being transferred. Currently, the client always retains the response object in a class property, which results in doubling the memory usage.

Solution:
In this pull request, I’ve made the preservation of the response object optional. This change reduces memory consumption by preventing the client from unnecessarily holding the response object in memory, especially for large file transfers.

Also: Error catching was refactored, test added, lib version updated

@rosh-n-ix
Copy link
Author

@iansparks @glow-mdsol
take a look please

@isparks-tg
Copy link

Looks good to me.

@rosh-n-ix
Copy link
Author

rosh-n-ix commented Feb 4, 2025

@jmann-mdsol @glow-mdsol @isparks-tg @isparks-tg
Could you please approve and merge this

Copy link

@isparks-tg isparks-tg left a comment

Choose a reason for hiding this comment

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

Ok with me.

@isparks-tg
Copy link

@rosh-n-ix can approve but not my place to merge. Defer to someone at Medidata, @glow-mdsol ?

Copy link
Member

@glow-mdsol glow-mdsol left a comment

Choose a reason for hiding this comment

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

LGTM

@glow-mdsol
Copy link
Member

glow-mdsol commented Feb 4, 2025

Let me do a quick PR to resolve the issue with CI, then we can merge that and get this pulled in

@glow-mdsol glow-mdsol mentioned this pull request Feb 4, 2025
@glow-mdsol
Copy link
Member

Ok, should be good to update from develop and then we can get this released

@rosh-n-ix rosh-n-ix force-pushed the feature/optional_last_response_preserving branch from 89d85a1 to 85726f0 Compare February 4, 2025 13:40
@rosh-n-ix
Copy link
Author

@glow-mdsol done! Thank you

@glow-mdsol glow-mdsol merged commit aa32295 into mdsol:develop Feb 4, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants