admission: ensure GetPebbleMetrics is not called after StoreGrantCoor… #146301
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/util/admission/grant_coordinator.go line 284 at r1 (raw file):
// permitted. sgc.closeCh <- struct{}{} close(sgc.closeCh)
[nit] do we still need to close? Feels strange to do both
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/util/admission/grant_coordinator.go line 284 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] do we still need to close? Feels strange to do both
Figured it is the clean thing to do. If somehow StoreGrantCoordinators.close gets called twice due to a bug, sending on a closed channel will panic. Of course, there are other ways to accomplish this. I don't have a strong opinion.
There was a problem hiding this comment.
Reviewable status:
pkg/util/admission/grant_coordinator.go line 284 at r1 (raw file):
Previously, sumeerbhola wrote…
Figured it is the clean thing to do. If somehow
StoreGrantCoordinators.closegets called twice due to a bug, sending on a closed channel will panic. Of course, there are other ways to accomplish this. I don't have a strong opinion.
Good point. Maybe add a comment above close.
…dinators.close Fixes cockroachdb#140454, cockroachdb#144172 Epic: none Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/util/admission/grant_coordinator.go line 284 at r1 (raw file):
Previously, RaduBerinde wrote…
Good point. Maybe add a comment above close.
Done
|
bors r=RaduBerinde |
…dinators.close
Fixes #140454, #144172
Epic: none
Release note: None