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

Synchronous reads for improved performance for large payloads #134

Merged
merged 7 commits into from
Jan 29, 2023

Conversation

bhugot
Copy link
Contributor

@bhugot bhugot commented Jan 2, 2023

…on because of mssql client async bug sqlcli

It should be easy to switch back when the fix is done
It should also fix #126

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I'd like to reduce its scope a little so that it only impacts execution-related SQL calls. I'd also like it if we can try to reduce some of the duplication.

src/DurableTask.SqlServer/SqlUtils.cs Outdated Show resolved Hide resolved
src/DurableTask.SqlServer/SqlUtils.cs Outdated Show resolved Hide resolved
src/DurableTask.SqlServer/SqlUtils.cs Outdated Show resolved Hide resolved
@bhugot
Copy link
Contributor Author

bhugot commented Jan 27, 2023

@cgillum a new version is coming as my fix was incorrect the performance issue what on the reader and not on ExecuteReaderAsync I let a first run to show the test performance (on my computer it was around 1 min 40 and after the fix it was 12sec)

@cgillum
Copy link
Member

cgillum commented Jan 28, 2023

@bhugot I just verified your fix on my machine: your stress test takes 1.1 minutes without the fix and is 4.5 seconds with your fix. That's pretty incredible!

@bhugot
Copy link
Contributor Author

bhugot commented Jan 28, 2023

:)

@cgillum cgillum changed the title duplicate Retry and ExecuteSprocAndTraceAsync for synchronous executi… Synchronous reads for improved performance for large payloads Jan 28, 2023
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

@bhugot thanks for this great PR! I added some minor changes to clean up a few things. Let me know if you have any questions or concerns about my changes.

@bhugot
Copy link
Contributor Author

bhugot commented Jan 29, 2023

It's ok for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants