-
Notifications
You must be signed in to change notification settings - Fork 52
Enable HttpPostRequestCallback to fail requests #124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9e02c27
29139e1
93ba816
084fd52
00fdaf6
c32d55b
cb1ed41
ae5f5fe
c6f7b49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -378,6 +378,18 @@ and then reference identifier `rest-lookup-logger` in the HTTP lookup DDL proper | |
is provided. | ||
|
||
|
||
- Callback Errors: | ||
|
||
Throw a [FailedRequestException](src/main/java/com/getindata/connectors/http/FailedRequestException.java) to indicate a | ||
failed request. | ||
|
||
This allows control over the connector's behavior when an HTTP response does not meet your expectations | ||
whether based on the response body or headers. | ||
|
||
Currently, the only side effect is to incremenet the [numRecordsSendErrors counter](https://github.com/getindata/flink-http-connector?tab=readme-ov-file#http-sink-2), as the connector does not | ||
support retries yet. However, once retry functionality is implemented, this will allow users to specify if requests should be retried. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the connector does support retries |
||
|
||
|
||
## HTTP status code handler | ||
Http Sink and Lookup Source connectors allow defining list of HTTP status codes that should be treated as errors. | ||
By default all 400s and 500s response codes will be interpreted as error code. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
package com.getindata.connectors.http; | ||
|
||
/** | ||
* Exception thrown from a {@link HttpPostRequestCallback} | ||
* when a request should be considered as failed. | ||
* | ||
* <p>This exception is caught by the | ||
* {@link com.getindata.connectors.http.internal.sink.httpclient.JavaNetSinkHttpClient} | ||
* and {@link com.getindata.connectors.http.internal.table.lookup.JavaNetHttpPollingClient} | ||
*/ | ||
public class FailedRequestException extends Exception { | ||
public FailedRequestException(String message) { | ||
super(message); | ||
} | ||
|
||
public FailedRequestException(String message, Throwable cause) { | ||
super(message, cause); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
import org.apache.flink.table.data.RowData; | ||
import org.apache.flink.util.StringUtils; | ||
|
||
import com.getindata.connectors.http.FailedRequestException; | ||
import com.getindata.connectors.http.HttpPostRequestCallback; | ||
import com.getindata.connectors.http.internal.PollingClient; | ||
import com.getindata.connectors.http.internal.config.HttpConnectorConfigConstants; | ||
|
@@ -89,7 +90,14 @@ private Optional<RowData> processHttpResponse( | |
HttpResponse<String> response, | ||
HttpLookupSourceRequestEntry request) throws IOException { | ||
|
||
this.httpPostRequestCallback.call(response, request, "endpoint", Collections.emptyMap()); | ||
try { | ||
this.httpPostRequestCallback.call( | ||
response, request, "endpoint", Collections.emptyMap() | ||
); | ||
} catch (FailedRequestException e) { | ||
log.debug("FailedRequestException thrown by httpPostRequestCallback", e); | ||
return Optional.empty(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we not throw an Exception here? There is already precedence as this method already throws throws IOException. Why is this not log.error - the text says Error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same reason as below, I don't think there's a need to bubble up the exception, just treat this request as failed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code has changed in this area. Prior to this change all failures resulted in the job ending. It is possible to specify in the table config the list of status codes to count as ignore, retry, failed or successful. If the logic in the callback is around status codes, then we do not need this extra code change. If your callback is doing more sophisticated checking of the response payload, then this would make sense, the exception should be thrown if you want to fail the call, continue-on-error true will determine if the job carries on or not. I do not think we should swallow the Exception as this is not in line with the design of this connector. WDYT? |
||
} | ||
|
||
if (response == null) { | ||
return Optional.empty(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo incremenet