HTTP183: consolidate http configuration between sink and lookup #184
Conversation
src/main/java/com/getindata/connectors/http/internal/status/HttpResponseChecker.java
Show resolved
Hide resolved
|
|
||
| httpPostRequestCallback.call( | ||
| optResponse.orElse(null), sinkRequestEntry, endpointUrl, headerMap); | ||
| httpPostRequestCallback.call(optResponse.orElse(null), sinkRequestEntry, endpointUrl, headerMap); |
There was a problem hiding this comment.
I think it would be helpful to also include the responseChecker in the callback, but that should be done in another pull request. The HttpResponseChecker is critical to properly understand request response information, and could be valuable, especially with logging.
| } | ||
|
|
||
| public boolean isErrorCode(int httpStatusCode) { | ||
| return !isTemporalError(httpStatusCode) && !isSuccessful(httpStatusCode); |
There was a problem hiding this comment.
if we do checks like this can we ensure that the same status code cannot occur in multiple pla es - with an early config validation.
There was a problem hiding this comment.
The HttpResponseChecker does validate that the combination of success and ignore codes are not contained within temporal codes. This happens on creation during the validate call under this function https://github.com/getindata/flink-http-connector/pull/184/files/ea116a6e703edcbdcafe1a3e6b51bad53a4166bd#diff-c35b6c5d289edf15e9e5773c77632b091bc633e825a5da7112fb5d4434e5655cR73. Currently, with the lookup table, you can have ignore codes and success codes overlap, so a check there would potentially break backwards compability. Is that what check you are referring to?
|
|
||
| The sink categorizes HTTP responses into groups: | ||
| - Success codes (`gid.connector.http.sink.success-codes`): Expected successful responses. `1XX, 2XX, 3XX` are defaults | ||
| - Retry codes (`gid.connector.http.sink.retry-codes`): Transient errors that trigger automatic retries when using `at-least-once` delivery guarantee. `500, 503, 504` are defaults |
There was a problem hiding this comment.
500s should not be retried - these are internal server errors - not retriable codes. I do not think we should have any defaults.
There was a problem hiding this comment.
The 500 default was done to match the lookup table defaults. I'm okay with removing 500, but, should we do the same to lookup as well? (probably outside the bounds of this pull request) I'm not sure if we need the two to match exactly as the use case is a bit different
| - Ignored responses (`gid.connector.http.sink.ignored-response-codes`): Responses whose content is ignored but treated as successful. | ||
| - Error codes: Any response code not classified in the above groups. | ||
|
|
||
| Parameters support whitelisting and blacklisting: `2XX,404,!203` means all codes from 200-299, plus 404, except 203. |
There was a problem hiding this comment.
I suggest not using whitelisting and blacklisting as the language is questionable- I have made this change in the apache version.
There was a problem hiding this comment.
Okay, I will switch it to denylist & allowlist, but please let me know if you had another naming convention in my mind!
| | gid.connector.http.sink.error.code.exclude | optional | List of HTTP status codes that should be excluded from the `gid.connector.http.sink.error.code` list, separated with comma. | | ||
| | gid.connector.http.sink.error.code `DEPRECATED` | optional | List of HTTP status codes that should be treated as errors by HTTP Sink, separated with comma. | | ||
| | gid.connector.http.sink.error.code.exclude `DEPRECATED` | optional | List of HTTP status codes that should be excluded from the `gid.connector.http.sink.error.code` list, separated with comma. | | ||
| | gid.connector.http.sink.success-codes | optional | Comma separated http codes considered as success response. Use [1-5]XX for groups and '!' character for excluding. Defaults are `1XX,2XX,3XX` | |
There was a problem hiding this comment.
I think the default should be 200, as was the case for the lookup.
There was a problem hiding this comment.
1XX,2XX,3XX was done to preserve backwards compatibility to the existing/old configuration. I am okay with just 2XX, but should we place 1XX and 3XX into a default ignore code set?
|
|
||
| public enum ResponseItemStatus { | ||
| SUCCESS("success"), | ||
| TEMPORAL("temporal"), |
There was a problem hiding this comment.
I think RETRIABLE would be better than TEMPORAL.
There was a problem hiding this comment.
Okay! I used to TEMPORAL to match the lookup table's language. Eventually, we may want to change it their as well
2 participants
Description
consolidate http configuration between sink and lookupResolves
183PR Checklist