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

Performance Improvement #142

Merged
merged 6 commits into from
Oct 26, 2023
Merged

Performance Improvement #142

merged 6 commits into from
Oct 26, 2023

Conversation

a-teece
Copy link
Contributor

@a-teece a-teece commented Oct 26, 2023

  • Improved the PreciseDelay method to enhance accuracy and significantly decrease CPU load.
  • Fix percent completed message.
  • Handle "stopped" in more places to try and fix shutdown failures (still doesn't shutdown correctly after pressing ctrl-c when using ReplayConsumer).

@spaghettidba
Copy link
Owner

Great job! Thank you for your contributions!
One of the reasons why I did not close replay workers and the associated connection in the original code is the possibility that the connection holds an open transaction and does not have any commands in the queue. In this implementation I am afraid that the connection would get closed. Is this taken care of?

@a-teece
Copy link
Contributor Author

a-teece commented Oct 26, 2023

I haven't actually changed that. You were already closing / opening the connection with no regards to transactions.

All I have done is remove the delay for the "next" command in such a scenario. The reasoning is that a developer will likely

Open connection
run command
close connection

think time
think time
think time

Open connection
run command
close connection

So the replay should be trying to replicate the "think time", but not the time between Open connection/Run Command. This is likly tiny (so not really replicatable given accuracy challenges).

Theoretically if there is a SQL Transaction then the original code could not have closed/opened the connection. If it was a DTC transaction, which would allow the close/open (our app does this) then I don't believe the trace has captured that and none of the code replicates that. So I'm not sure if you have a transaction problem anyway.

@spaghettidba
Copy link
Owner

Ah I see. Maybe I was just thinking about it but I never coded it correctly.
Sorry about that. As you could see, the code is pretty complex (for me at least!) and there is so much to take care of...
Thanks again for your help! I hope this is getting closer to being helpful for you!

@spaghettidba spaghettidba merged commit b4f9401 into spaghettidba:master Oct 26, 2023
@a-teece
Copy link
Contributor Author

a-teece commented Oct 26, 2023

It is certainly being helpful now. We're comparing a workload of SQL on AWS EC2 vs Azure Managed Instance. Using this tool to prove the performance goals can be met of a migration.

@spaghettidba
Copy link
Owner

This is great news, thanks for the feedback!

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.

None yet

2 participants