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

sqlproxy: fix some small issues and tidy up files and comments #66369

Merged
merged 4 commits into from
Jun 12, 2021

Conversation

andy-kimball
Copy link
Contributor

See commit comments for details.

@andy-kimball andy-kimball requested a review from darinpp June 11, 2021 16:28
@andy-kimball andy-kimball requested a review from a team as a code owner June 11, 2021 16:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andy-kimball andy-kimball requested a review from a team as a code owner June 11, 2021 18:23
Copy link
Contributor

@darinpp darinpp left a comment

Choose a reason for hiding this comment

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

Looks good

@andy-kimball andy-kimball force-pushed the sqlproxy branch 2 times, most recently from e3d33d4 to e022795 Compare June 11, 2021 21:38
Do a bit of reorganization/tidying of files:

1. Remove mock files, which are never used.
2. Move tests into package directory they are testing. Avoid import cycles
   by qualifying their package name with "_test".
3. Move the TestDirectoryServer into the tenantdirsvr package and rename the
   NewTestDirectoryServer method to just be New.
4. Move certificate files into new testdata directory.

Release note: None
The tests were spewing out 150,000 warnings to the log when run because there
is no denylist file present. This commit changes the proxy to not create the
denylist service if no denylist file is provided.

Release note: None
Our coding standard is to keep comments within an 80 char limit, assuming two
characters per tab. This commit fixes up the code to conform to that.

Relase note: None
@andy-kimball andy-kimball force-pushed the sqlproxy branch 2 times, most recently from 7d7b21f to 46cb2d4 Compare June 12, 2021 02:49
1. updateMetricsAndSendErrToClient was called twice in proxyHandler.
2. Use GRPC NotFound error rather than error with text "not found".
3. Remove unnecessary code in ensureTenantEndpoint that was fetching endpoints
   before calling EnsureEndpoint.

Release note: None
@andy-kimball
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 12, 2021

Build succeeded:

@craig craig bot merged commit 2e3fca3 into cockroachdb:master Jun 12, 2021
@andy-kimball andy-kimball deleted the sqlproxy branch June 12, 2021 05:43
irfansharif added a commit to irfansharif/cockroach that referenced this pull request Jul 6, 2021
Fixes cockroachdb#67010. Also clean up some rotted gomock generation in sqlproxyccl
(mockgen usage was removed in cockroachdb#66369).

Release note: None
irfansharif added a commit to irfansharif/cockroach that referenced this pull request Jul 8, 2021
Fixes cockroachdb#67010. Also clean up some rotted gomock generation in sqlproxyccl
(mockgen usage was removed in cockroachdb#66369).

Release note: None
irfansharif added a commit to irfansharif/cockroach that referenced this pull request Jul 19, 2021
Fixes cockroachdb#67010. Also clean up some rotted gomock generation in sqlproxyccl
(mockgen usage was removed in cockroachdb#66369).

Release note: None
craig bot pushed a commit that referenced this pull request Jul 19, 2021
67226: range{feed,client},roachpb: standardize mock generation through bazel r=irfansharif a=irfansharif

Fixes #67010. Also clean up some rotted gomock generation in sqlproxyccl
(mockgen usage was removed in #66369).

Release note: None

67478: backupccl: fix settings registry leak during restore r=pbardea a=pbardea

Previously, every time a restore data processor was created, a callback
was added to the settings registry to update the size of the pool. This
could potentially lead to a large growth in registered callbacks that
close over the processors created.

This commit replaces that mechanism with a background poller that polls
the cluster setting every 30 seconds to see if the worker pool needs
updating.

Release note (bug fix): Fix a minor resource leak that occurs when a
RESTORE is run.

Co-authored-by: irfan sharif <[email protected]>
Co-authored-by: Paul Bardea <[email protected]>
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