Centralize HTTP client in StatusListService #29
Conversation
There was a problem hiding this comment.
Hello @Ogenbertrand, I have added these few comments.
Could you take a look?
| private final String authToken; | ||
| private final HttpClient httpClient; | ||
| private final CloseableHttpClient httpClient; | ||
| private final ObjectMapper objectMapper; |
There was a problem hiding this comment.
CloseableHttpClient holds connection pools and threads that must be explicitly released. Not closing it causes resource leaks. Could you implement an AutoCloseable or add a cleanup method?
| return HttpClients.custom() | ||
| .setDefaultRequestConfig(requestConfig) | ||
| .setRetryStrategy(retryStrategy) | ||
| .build(); |
There was a problem hiding this comment.
HTTP clients are expensive to create, and creating one per service instance is wasteful. You could consider using a shared HTTP client pool or a singleton pattern.
| logger.warnf("[Attempt %d/%d] Failed to send status: %d %s", | ||
| execCount, maxRetries, response.getCode(), response.getReasonPhrase()); | ||
| int status = response.getCode(); | ||
| return execCount <= maxRetries && (status >= 500); |
There was a problem hiding this comment.
Don't you think this is going to retry all IOExceptions, including non-retryable ones. What if we only retry on transient failures?
| }); | ||
| } catch (IOException | URISyntaxException e) { | ||
| if (e.getCause() instanceof StatusListServerException) { | ||
| throw (StatusListServerException) e.getCause(); |
There was a problem hiding this comment.
The current changes wrap StatusListServerException inside an IOException and then manually unwrap it later. Doesn't this add unnecessary complexity and make the exception flow harder to follow?
What if we just throw StatusListServerException directly when the status code indicates a failure? This will simplify the try/catch logic.
2 participants
Previos State: There are two different implementations for communicating with the external status list server:
The Problem: This creates code duplication, inconsistencies in error handling and configuration, and adds an unnecessary dependency.
Required Action: All HTTP logic should be centralized within the StatusListService. The StatusListProtocolMapper should be refactored to call methods on the StatusListService instead of making its own HTTP requests.
Closes: #26