CDF-26312: GenericClient: add wrapSttpBackend argument #904
Conversation
In addition to baseSttpBackend allow settign wrapSttpClient. GenericClient inserts auth wrapper in between and before setting retrying wrapper as base client would mean each retry is using same auth headers, which could expire for long and numerous retries. With new argument caller can move retrying wrapper from base client to wrapping argument and get up-to-date auth on each attempt. [CDF-26312]
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a wrapSttpBackend argument to GenericClient, allowing for more flexible customization of the STTP backend, particularly for handling retries with token refreshes. The implementation is sound and correctly applies the wrapper around the authentication backend. I've found one minor issue where cdfVersion is not passed when creating the RequestSession for token inspection, which could lead to issues in multi-cluster environments. I've left a specific comment with a suggestion to fix it.
| lazy val token = | ||
| new Token[F](RequestSession(applicationName, uri, sttpBackend, authProvider, clientTag)) | ||
| new Token[F]( | ||
| RequestSession(applicationName, uri, sttpBackend, wrapSttpBackend, authProvider, clientTag) |
There was a problem hiding this comment.
The RequestSession created for token is missing the cdfVersion parameter. This means that requests made by the token instance (e.g., token.inspect()) will not include the cdf-version header, which could lead to incorrect request routing in multi-cluster environments. Please add cdfVersion to the RequestSession constructor call to ensure consistent behavior with other API requests.
| RequestSession(applicationName, uri, sttpBackend, wrapSttpBackend, authProvider, clientTag) | |
| RequestSession(applicationName, uri, sttpBackend, wrapSttpBackend, authProvider, clientTag, cdfVersion) |
There was a problem hiding this comment.
will look into it separately
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #904 +/- ##
=======================================
Coverage 84.73% 84.73%
=======================================
Files 99 99
Lines 2738 2738
Branches 211 184 -27
=======================================
Hits 2320 2320
Misses 418 418
🚀 New features to boost your workflow:
|
|
This pull request seems a bit stale.. Shall we invite more to the party? |
In addition to baseSttpBackend allow settign wrapSttpClient.
GenericClient inserts auth wrapper in between and before setting
retrying wrapper as base client would mean each retry is using same
auth headers, which could expire for long and numerous retries.
With new argument caller can move retrying wrapper from base client to
wrapping argument and get up-to-date auth on each attempt.
CDF-26312