CF RAY improvements#78
Conversation
|
adams85
left a comment
adams85
left a comment
There was a problem hiding this comment.
Nothing blocker, just some minor observations:
| } catch (Exception e) { | ||
| if(fetcher != null) fetcher.close(); | ||
| if(monitor != null) monitor.close(); | ||
| this.logger.error(0, "ConfigCatClient initialization failed.", e); |
There was a problem hiding this comment.
Since ConfigCatClient.get throws an exception with the same message, logging this issue looks somewhat redundant. (I mean you can't really miss the exception anyway.)
| this.logger.error(0, "ConfigCatClient initialization failed.", e); |
| FormattableLogMessage message = ConfigCatLogMessages.getFetchFailedDueToUnexpectedError(cfRayId); | ||
| logger.error(1103, message, e); | ||
| result.complete(FetchResponse.failed(message + " " + e.getMessage(), false, null)); | ||
| fetchResponse = FetchResponse.failed(message + " " + e.getMessage(), false, cfRayId); |
There was a problem hiding this comment.
There's a minor inconsistency here compared to the Java SDK, which doesn't append e.getMessage() to the message returned by getFetchFailedDueToUnexpectedError.
I'm not sure which approach is the better.
The best solution is to store the exception in the FetchResponse object and expose it to the user via RefreshResult. But this is the subject of another ticket.
So we can leave this as is for now, I guess.
2 participants
Describe the purpose of your pull request
Related issues (only if applicable)
How to test? (only if applicable)
Security (only if applicable)
Requirement checklist (only if applicable)