fix: Removes unnecessary error logging for heartbeat since this is ex… #4809
Conversation
| ) { | ||
| execute(HttpMethod.POST, path, jsonEntity, (resp, vcf) -> { | ||
| }).exceptionally(t -> { | ||
| log.error("Unexpected exception in async request", t); | ||
| if (expectFailures) { |
There was a problem hiding this comment.
This precedes even this change, but I feel we should throw an exception instead of returning null? and let the caller decide whether to log or not.. I am wondering if we should pass a flag down into a common class like KsqlTarget
There was a problem hiding this comment.
On second thought, guess the class itself is aware of different endpoints.. more about this method.. can it throw the exception and let the caller decide
There was a problem hiding this comment.
I'd agree with Vinoth here - methods like postAsyncHeartbeatRequest and postAsyncLagReportingRequest should return a CompletableFuture, which allows the caller to decide if the care about the response and if they do, what to do with it.
There was a problem hiding this comment.
I was originally just keeping with an internal implementation that might be reusable for "fire and forget" type methods, but I'm also fine with making this a bit more general and letting callers handle the response/exception.
Changed to return CompletableFuture<RestResponse<HeartbeatResponse>> which can now be handled by the caller, including handling the exceptional case.
ksqldb-rest-client/src/main/java/io/confluent/ksql/rest/client/KsqlTarget.java
Outdated
Show resolved
Hide resolved
| ) { | ||
| final CompletableFuture<ResponseWithBody> vcf = | ||
| execute(httpMethod, path, requestBody, responseHandler); | ||
| return vcf.thenApply(response -> KsqlClientUtil.toRestResponse(response, path, mapper)); |
There was a problem hiding this comment.
Is thenApply asynchronous? We don't want to block for the response. In fact, HeartbeatAgent doesn't even care about the response.
There was a problem hiding this comment.
Discussed offline, as long as we don't call get on the future , all other callbacks are non-blocking so we are good
There was a problem hiding this comment.
New structuring LGTM. I ll leave to @vpapavas for the final pass
| .exceptionally(t -> { | ||
| // We send heartbeat requests quite frequently and to nodes that might be down. We don't | ||
| // want to fill the logs with spam, so we debug log exceptions. | ||
| LOG.debug("Exception in async request", t); |
There was a problem hiding this comment.
nit: async heartbeat request?
| getTarget(target, authHeader).postAsyncLagReportingRequest(lagReportingMessage); | ||
| getTarget(target, authHeader).postAsyncLagReportingRequest(lagReportingMessage) | ||
| .exceptionally(t -> { | ||
| LOG.error("Unexpected exception in async request", t); |
There was a problem hiding this comment.
Don't we want to debug log this as well?
There was a problem hiding this comment.
Looking at the code, it seems like all of the hosts start off in a non-alive state and if the hoststatus callback happens at the wrong time, I think lag reporting could hit the same issue as well.
Making it debug logging as well.
…pected
Description
Disables error logging for heartbeat requests since these happen quite often and deliberately are done with nodes that might be down. During startup especially (and possibly other situations as well) this leads to tons of log spam. Since we have other mechanisms like the hearbeatAgent which log periodically when hosts are considered down, this isn't necessary.
Fixes #4807
Testing done
Ran tests.
Reviewer checklist