Use airlift json codec in http event listener, fixing airlift/airlift#983 #10843
Conversation
| this.queryCompletedEventJsonCodec = requireNonNull(queryCompletedEventJsonCodec, "query complete event json codec is null"); | ||
| this.queryCreatedEventJsonCodec = requireNonNull(queryCreatedEventJsonCodec, "query create event json codec is null"); | ||
| this.splitCompletedEventJsonCodec = requireNonNull(splitCompletedEventJsonCodec, "split complete json codec is null"); |
There was a problem hiding this comment.
use variable name in error string (queryCompletedEventJsonCodec is null, ...)
|
Thanks very much for the review? How would like the commits be ordered? Would the following work?
|
makes sense
This one can be split into two |
mosiac1
force-pushed
the
370_http_events_fix
branch
from
a913c59 to
6a0c8b7
Compare
|
@losipiuk Hello, I reordered the commits as we discussed! CI is failing because of iceberg, could we please rerun it? |
Fix issue airlift/airlift#983 Update server disconnect test to use min threads to test for this issue Cleanup tests
mosiac1
force-pushed
the
370_http_events_fix
branch
from
6a0c8b7 to
36089ac
Compare
| } | ||
| else { | ||
| log.error("QueryId = \"%s\", attempt = %d/%d, URL = %s | Ingest server responded with code %d, fatal error", | ||
| queryId, attempt + 1, config.getRetryCount() + 1, request.getUri().toString(), |
There was a problem hiding this comment.
nit: toString() in uri.toString() not needed
| queryId, attempt + 1, config.getRetryCount() + 1, request.getUri().toString()); | ||
| else { | ||
| log.debug("QueryId = \"%s\", attempt = %d/%d, URL = %s | Query event delivered successfully", | ||
| queryId, attempt + 1, config.getRetryCount() + 1, request.getUri().toString()); |
There was a problem hiding this comment.
nit: toString() in uri.toString() not needed
|
Merged. Sorry for delay @mosiac1 (I forgot about this one). |
|
Thank you, no worries about the delay |
A bug in airlift described in this issue airlift/airlift#983 affects the event listener, causing it to leak threads and timeout all requests. This fixes the effect of the issue by not using the dangerous class but not the source of the issue.
I also changed the
testServerDisconnectShouldRetryto cover the issue above. With the old setup, this would fail because the http-client's threads would go into deadlock.Other changes: replace the name
querylogwithhttpeventlisteneroreventlistenerin tests, cleanup logs.