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

Fixes DBConnection checkout related flakiness #2731

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

gagandeepb
Copy link
Contributor

@gagandeepb gagandeepb commented Jul 1, 2024

Description

Some flakiness/test failures were being observed locally due to processes timing out while waiting to checkout a DB connection during test runs. This test config change tries to fix the issue. From https://hexdocs.pm/db_connection/DBConnection.html#start_link/2-queue-config :

Our goal is to wait at most :queue_target for a connection. If all connections checked out during a 
:queue_interval takes more than :queue_target, then we double the :queue_target. If checking out
 connections take longer than the new target, then we start dropping messages.

Fixes # (issue)

Did you add the right label?

Remember to add the right labels to this PR.

  • DONE

How was this tested?

Describe the tests that have been added/changed for this new behavior.

  • DONE

Did you update the documentation?

Remember to ask yourself if your PR requires changes to the following documentation:

Add a documentation PR or write that no changes are required for the documentation.

  • DONE

Some flakiness/test failures were being observed due to processes timing out while waiting to checkout the DBConnection during test runs.  This config changes tries to fix the issue. From https://hexdocs.pm/db_connection/DBConnection.html#start_link/2-queue-config :

Our goal is to wait at most :queue_target for a connection. If all connections checked out during a :queue_interval takes more than :queue_target, then we double the :queue_target. If checking out connections take longer than the new target, then we start dropping messages.
@gagandeepb gagandeepb added chore elixir Pull requests that update Elixir code labels Jul 1, 2024
@gagandeepb gagandeepb added enhancement New feature or request bug Something isn't working and removed chore enhancement New feature or request labels Jul 1, 2024
@gagandeepb gagandeepb requested review from a team, jagabomb, EMaksy, dottorblaster, jamie-suse, nelsonkopliku, CDimonaco, arbulu89 and janvhs and removed request for a team, jagabomb and EMaksy July 1, 2024 16:23
@gagandeepb gagandeepb marked this pull request as ready for review July 1, 2024 16:33
@gagandeepb gagandeepb requested a review from balanza July 1, 2024 16:40
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Awesome! It is way less flaky than without the proposed change.

I still get now and then Connection Errors, tho, but way less frequently.

Maybe some further tweaking might reduce even more the risk, but I am already happy.

Thanks!

@@ -12,6 +12,8 @@ config :trento, Trento.Repo,
hostname: "localhost",
port: 5433,
pool: Ecto.Adapters.SQL.Sandbox,
queue_target: 200,
queue_interval: 2000,
Copy link
Member

Choose a reason for hiding this comment

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

lgtm, but I guess this have also some kind of impact on the time of the ci/cd run, right ?

Copy link
Contributor Author

@gagandeepb gagandeepb Jul 2, 2024

Choose a reason for hiding this comment

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

The tests were timing out while waiting for a db connection to become available. The additional delay is in the order of milliseconds (at most a few seconds). That's locally. The CI impact should be negligible, if any, in my understanding

@gagandeepb gagandeepb merged commit 39214d4 into main Jul 2, 2024
75 of 76 checks passed
@gagandeepb gagandeepb deleted the fix-tests-dbconn-checkout-flakiness branch July 2, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working elixir Pull requests that update Elixir code
Development

Successfully merging this pull request may close these issues.

5 participants