physical: use permitpool from go-secure-stdlib#29331
physical: use permitpool from go-secure-stdlib#29331stevendpclark merged 4 commits intohashicorp:mainfrom
Conversation
johanbrandhorst
force-pushed
the
jbrandhorst-use-permitpool
branch
from
29e9c88 to
aca6d4f
Compare
There was a problem hiding this comment.
The permitpool now requires a context argument, so that it can unblock in the case of context cancellations. I have weaved this into the code using existing contexts where possible, otherwise using context.Background().
johanbrandhorst
force-pushed
the
jbrandhorst-use-permitpool
branch
from
aca6d4f to
ea2afd8
Compare
johanbrandhorst
force-pushed
the
jbrandhorst-use-permitpool
branch
from
ea2afd8 to
39d591e
Compare
stevendpclark
left a comment
stevendpclark
left a comment
There was a problem hiding this comment.
Overall +1, I just have two comments/questions before approval.
| // Factory is the factory function to create a physical backend. | ||
| type Factory func(config map[string]string, logger log.Logger) (Backend, error) | ||
|
|
||
| // PermitPool is used to limit maximum outstanding requests |
There was a problem hiding this comment.
Should this not be marked as deprecated and the existing NewPermitPool return the shared version for at least some backwards compatibility since this is the SDK package?
Touchy as the Acquire method has changed 🤔
There was a problem hiding this comment.
existing NewPermitPool return the shared version sorry late in the day comment. I meant wrap the new shared implementation within itself and delegate to it internally from the existing methods.
There was a problem hiding this comment.
Great question, I didn't clock that this was a package users may depend on. I'm sort of worried that adding a wrapper here will just make users in this repo keep using it, but I suppose that's a risk we have to take. I'll add a wrapper.
There was a problem hiding this comment.
I added a wrapper which should be code-compatible, please take a look
stevendpclark
left a comment
stevendpclark
left a comment
There was a problem hiding this comment.
👍 Looks good to me, thanks for doing this.
2 participants
The
PermitPooltype is being moved to go-secure-stdlib to be shared with Boundary.sdk/physical: use permitpool from go-secure-stdlib
physical: use permitpool from go-secure-stdlib