Skip to content

Conversation

@dmivankov
Copy link
Contributor

@dmivankov dmivankov commented Nov 19, 2025

Currently Client accepts base client and wraps it with tracing&auth
headers. So to add retries the flow was making base sttp client retrying.
This may run into issue of expiring auth header, auth provider would've
refreshed it if it was called, but all retrying happens below it.

Now adding optional parameter to customize sttp client by wrapping,
intent is for calller to add retries by wrapping making each try request
auth again and have a chance to refresh it if it expires soon

CDF-26312

…ter retries)

Currently Client accepts base client and wraps it with tracing&auth
headers. So to add retries the flow was making base sttp client retrying.
This may run into issue of expiring auth header, auth provider would've
refreshed it if it was called, but all retrying happens below it.

Now adding optional parameter to customize sttp client by wrapping,
intent is for calller to add retries by wrapping making each try request
auth again and have a chance to refresh it if it expires soon

[CDF-26312]
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a wrapSttpBackend parameter to RequestSession to allow for more flexible customization of the sttp client, particularly for implementing retry logic that re-authenticates on each attempt. While the change in RequestSession is correct, the new functionality is not exposed to the client's users because GenericClient has not been updated to accept and pass this new parameter. This is a critical issue that prevents the feature from being used.

cdfVersion: Option[String] = None,
tags: Map[String, Any] = Map.empty
tags: Map[String, Any] = Map.empty,
wrapSttpBackend: SttpBackend[F, _] => SttpBackend[F, _] = identity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The addition of wrapSttpBackend to RequestSession is a good approach to allow for more flexible client customization. However, this new functionality is not currently exposed to the end-user.

The GenericClient is responsible for creating the RequestSession, but it has not been updated to accept or pass along the wrapSttpBackend function. As a result, the RequestSession will always be created with the default identity wrapper, and the new feature will be unusable.

To complete this feature, you'll need to thread the wrapSttpBackend parameter through GenericClient's constructors and factory methods, and use it when instantiating RequestSession.

@dmivankov dmivankov marked this pull request as draft November 19, 2025 08:33
dmivankov added a commit to cognitedata/cdp-spark-datasource that referenced this pull request Nov 19, 2025
Looks like due to retries of slow requests it can be a while until a refresh attempt

Fix to check invalidation on each retry will take a little more time due to interface changes etc (a related draft cognitedata/cognite-sdk-scala#896)

For now let's just increase the refresh safety margin

[CDF-26316]
dmivankov added a commit to cognitedata/cdp-spark-datasource that referenced this pull request Nov 20, 2025
Looks like due to retries of slow requests it can be a while until a refresh attempt

Fix to check invalidation on each retry will take a little more time due to interface changes etc (a related draft cognitedata/cognite-sdk-scala#896)

For now let's just increase the refresh safety margin

[CDF-26316]
@dmivankov dmivankov closed this Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants