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

sql: initialize with a limited number of table leases #29081

Closed
wants to merge 1 commit into from

Conversation

vivekmenezes
Copy link
Contributor

This will prevent initial requests hitting a server from
blocking on lease acquisition.

related to #23510

Release note: Fixed slowness caused by table lease acquisition
at startup.

@vivekmenezes vivekmenezes requested review from dt and a team August 27, 2018 03:20
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@vivekmenezes
Copy link
Contributor Author

@asubiotto perhaps you have some thoughts on how I might fix the TODO on this PR. I have this initialization method that is running some internal SQL on a system table, and would like to have it reach completion before a node receives traffic. But I worry that we have a chicken and egg situation where if all nodes wait to serve traffic they will not be able to serve the internal SQL request.

This will prevent initial requests hitting a server from
blocking on lease acquisition.

related to cockroachdb#23510

Release note: Fixed slowness caused by table lease acquisition
at startup.
@asubiotto
Copy link
Contributor

The only thing I can think of is to add something to

serveMode := s.admin.server.serveMode.get()
that returns that the server is ready to accept client traffic only when this lease acquisition has been performed. Although this won't work if the client is not observing the /health?ready=1 endpoint (but maybe that's the expectation?) @bdarnell or @a-robinson might have a better idea.

@dt
Copy link
Member

dt commented Aug 31, 2018

I think it is okay to accept traffic before this runs -- blocking and queuing connections on it will mean they see just as much, if not more, latency than just letting them hit a cold cache.

Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

I agree with @dt, just let the node start up and let anything that comes in before the lease acquisition finishes block to get the lease.

@tbg tbg added the X-noremind Bots won't notify about PRs with X-noremind label Jun 19, 2019
@dt dt closed this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-noremind Bots won't notify about PRs with X-noremind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants