-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
server: remove contention event registry from baseStatusServer #75835
server: remove contention event registry from baseStatusServer #75835
Conversation
37a26db
to
4a4ff9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng)
pkg/server/tenant.go, line 169 at r1 (raw file):
args.rpcContext, args.stopper, ) contentionRegistry := contention.NewRegistry()
nit: why not squash these two lines into one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Azhng)
Previously, baseStatusServer holds a reference to contention.Registry. This reference of contention.Registry is used to power the /ListContentionEvents endpoint. However, this is not ideal for two reasons: 1. baseStatusServer already holds a reference to *server.SQLServer, which in turn contains contention.Registry through its ExecutorConfig field. This means that there's no good reason to have another field in baseStatusServer to hold this additional reference. 2. The ongoing contention event registry work will make contention registry depend on status server to perform transaction ID resolution protocol. As it stand today, the status server's construction depends on the creation of contention.Registry. By introducing transaction ID resolution protocol into contention.Registry, we will be introducing a cyclical reference, which can lead to ugly API design. This commit removes the baseStatusServer's reference on contention.Registry, and instead directly fetching from executor config. Release note: None
4a4ff9a
to
39690fc
Compare
TFTR! bors r+ |
Build failed (retrying...): |
Build failed (retrying...): |
Build succeeded: |
Previously, baseStatusServer holds a reference to contention.Registry.
This reference of contention.Registry is used to power the
/ListContentionEvents endpoint. However, this is not ideal for two
reasons:
which in turn contains contention.Registry through its ExecutorConfig
field. This means that there's no good reason to have another field
in baseStatusServer to hold this additional reference.
registry depend on status server to perform transaction ID resolution
protocol. As it stand today, the status server's construction depends
on the creation of contention.Registry. By introducing transaction ID
resolution protocol into contention.Registry, we will be introducing
a cyclical reference, which can lead to ugly API design.
This commit removes the baseStatusServer's reference on
contention.Registry, and instead directly fetching from executor config.
Release note: None