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

ccl/sqlproxyccl: fixes flake on TestDirectoryConnect #101864

Merged

Conversation

jaylim-crl
Copy link
Collaborator

Fixes #76839, #71365.

This commit rewrites the test to use the static directory server instead of the dynamic one. The old one is susceptible to GRPC port reuse and deadlock issues. With this change, only directory_cache_test.go is relying on the dynamic directory server. Once we rewrite that, we can remove the entire test directory implementation.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jaylim-crl jaylim-crl force-pushed the jay/230419-fix-test-directory-connect branch 2 times, most recently from 68dab88 to 5dea502 Compare April 19, 2023 20:53
@jaylim-crl
Copy link
Collaborator Author

Added a dummy commit to figure out why make lint in Bazel Essential CI (Cockroach) is failing.

Fixes cockroachdb#76839 and cockroachdb#71365.

This commit rewrites the test to use the static directory server instead of
the dynamic one. The old one is susceptible to GRPC port reuse and deadlock
issues. With this change, only directory_cache_test.go is relying on the
dynamic directory server. Once we rewrite that, we can remove the entire test
directory implementation.

Release note: None
@jaylim-crl jaylim-crl force-pushed the jay/230419-fix-test-directory-connect branch from ddf4a65 to f45ac17 Compare April 20, 2023 13:43
@jaylim-crl jaylim-crl requested a review from jeffswenson April 20, 2023 13:43
@jaylim-crl jaylim-crl marked this pull request as ready for review April 20, 2023 13:43
@jaylim-crl jaylim-crl requested review from a team as code owners April 20, 2023 13:43
@jaylim-crl
Copy link
Collaborator Author

The make lint failure seems to be an already existing issue: https://cockroachlabs.slack.com/archives/CJ0H8Q97C/p1681997780769209 (unrelated to this PR).

Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

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

LGTM

@jaylim-crl
Copy link
Collaborator Author

TFTR!

bors r=JeffSwenson

@craig
Copy link
Contributor

craig bot commented Apr 21, 2023

Build succeeded:

@craig craig bot merged commit 0ee74ef into cockroachdb:master Apr 21, 2023
@jaylim-crl jaylim-crl deleted the jay/230419-fix-test-directory-connect branch April 21, 2023 13:45
@jeffswenson
Copy link
Collaborator

blathers backport 23.1

@blathers-crl
Copy link

blathers-crl bot commented Jul 7, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from f45ac17 to blathers/backport-release-23.1-101864: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

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.

ccl/sqlproxyccl: TestDirectoryConnect failed
3 participants