Skip to content

Conversation

@newtork
Copy link
Contributor

@newtork newtork commented Nov 18, 2025

Context

Related #1004

  • I opened this PR to document our decision for approval / dismissal. In case this comes up again in the future.
  • We aligned to keep the current behavior of HttpClient5 but add logging to it.

Log message written to debug:

cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5FactoryTest.java

[main] DEBUG com.sap.cloud.sdk.cloudplatform.connectivity.DefaultApacheHttpClient5Factory - Retrying request for destination null due to response 429. Retry attempt #1.
[main] DEBUG com.sap.cloud.sdk.cloudplatform.connectivity.DefaultApacheHttpClient5Factory - Retrying request for destination null due to response 503. Retry attempt #1.

@newtork newtork added do not merge Pull request must not be merged please review Request to review a pull request labels Nov 18, 2025
@newtork newtork changed the title chore: Disable HttpClient5 Retry-Strategy for 429 and 503 responses chore: Enhance HttpClient5 Retry-Strategy for 429 and 503 responses Nov 20, 2025
@newtork newtork added please merge Request to merge a pull request and removed do not merge Pull request must not be merged labels Nov 20, 2025
if( retry ) {
final String msg =
"Retrying {} request for destination {} to {} due to exception \"{}\". Retry attempt #{}.";
log.debug(msg, req.getMethod(), destination, req.getRequestUri(), exception.getMessage(), execCount);
Copy link
Member

Choose a reason for hiding this comment

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

should be at least warn, maybe even error

Suggested change
log.debug(msg, req.getMethod(), destination, req.getRequestUri(), exception.getMessage(), execCount);
log.warn(msg, req.getMethod(), destination, req.getRequestUri(), exception.getMessage(), execCount);

final boolean retry = super.retryRequest(req, exception, execCount, context);
if( retry ) {
final String msg =
"Retrying {} request for destination {} to {} due to exception \"{}\". Retry attempt #{}.";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Retrying {} request for destination {} to {} due to exception \"{}\". Retry attempt #{}.";
"Retrying {} request to {} due to exception \"{}\". Retry attempt #{}.";

since destination can be null and may not add much information if the URL is absolute (is it?). Alternatively, explicit null check to avoid strange looking log?

Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

is there a way to print out the wait duration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

please merge Request to merge a pull request please review Request to review a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants