Add TranslateKey to PartitionedRateLimiter #69407
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @mangod9 Issue DetailsPart of #65400 Main question is around lifetime. Should dispose dispose the inner limiter? Should it be an option?
|
|
I'm not an expert in this area. @kouvel, could you please review this change or suggest someone who can? |
|
@halter73 will be reviewing this |
|
Thanks Brennan for clarifying. I'm just asking as this change continues popping up on our stale PR radar so we need to evaluate what PR's are still being worked on vs. which ones have been abandoned. |
| // REVIEW: Do we want to have an option to dispose the inner limiter? | ||
| // Should the default be to dispose the inner limiter and have an option to not dispose it? | ||
| // See Stream wrappers like SslStream for prior-art | ||
| return new TranslatingLimiter<TResource, TOuter>(this, keyAdapter); |
There was a problem hiding this comment.
Good point. StreamWriter does the same thing with leaveOpen, so there's definitely precedent here. I think this is the more convenient behavior considering how this API is likely to be used. Keeping around the original PartitionedRateLimiter just to track it for disposal is annoying. Do we like just adding a bool parameter?
public PartitionedRateLimiter<TOuter> TranslateKey<TOuter>(Func<TOuter, TResource> keyAdapter, bool leaveOpen = false);We could add an options type if we think we're going to make this even more configurable in the future, but I think that's probably excessive.
For now, without a leaveOpen option, I think we have to default to not taking ownership like you have since that's more flexible although it's less convenient.
There was a problem hiding this comment.
Do we like just adding a
boolparameter?
I think so. I am unable to think of any other potential future options at this time. The prior-art with Stream wrappers and StreamWriter will be helpful when advocating for an API change.
Edit: #72551
BrennanConroy
force-pushed
the
brecon/translate
branch
from
af27444
to
68de958
Compare
|
Test failure is a known issue, unrelated to this change. |
Part of #65400
Main question is around lifetime. Should dispose dispose the inner limiter? Should it be an option?