fix: Disable retries on successful requests

[EvaSDK]

By HTTP standards, it makes no sense to retry a successful query.
This is also generating extra log lines that might be confusing as you expect the minimum after a successful query.
Might want to make it configurable if an API is expected to request retry on successful HTTP status code.

[danieljimeneznz]

I prefer early returns 😄

Suggested change

	if(Response.Status.Family.familyOf(statusCode) != Response.Status.Family.SUCCESSFUL){
	RetryIndicator retryIndicator = getResponseIndicator();
	log.info("Retry Indicator: {}", retryIndicator);
	if(retryIndicator == RetryIndicator.UNKNOWN
	&& CALLBACK_API_DOWN_HTTP_STATUS_CODE.contains(statusCode)){
	throw new ApiRequestErrorException("Unable to connect to callback API: "
	+ " received status: " + statusCode, kafkaRecord);
	} else if (retryIndicator.shouldRetry) {
	throw new ApiResponseErrorException("Received response retry=true", kafkaRecord);
	}
	if (Response.Status.Family.familyOf(statusCode) == Response.Status.Family.SUCCESSFUL) {
	return
	}
	RetryIndicator retryIndicator = getResponseIndicator();
	log.info("Retry Indicator: {}", retryIndicator);
	if (retryIndicator == RetryIndicator.UNKNOWN
	&& CALLBACK_API_DOWN_HTTP_STATUS_CODE.contains(statusCode)){
	throw new ApiRequestErrorException("Unable to connect to callback API: "
	+ " received status: " + statusCode, kafkaRecord);
	} else if (retryIndicator.shouldRetry) {
	throw new ApiResponseErrorException("Received response retry=true", kafkaRecord);
	}

[kedarkrishnan]

The connector is designed to retry based on the flag returned in the response.
Check README - Exception strategies
"The exception strategy is triggered if the response send back by the callback url had a retry flag set to true."

The Idea here was to keep the retry strategy decoupled from the HTTP Status. Hence even for HTTP Success we check the retry flag.
This change breaks that.

[EvaSDK]

The Idea here was to keep the retry strategy decoupled from the HTTP Status. Hence even for HTTP Success we check the retry flag. This change breaks that.

I see. It is a bit confusing when later describing the default according to HTTP status code and no field always means retry=True. Neither this default nor the HTTP status code is configurable so I implemented what I expected normal given a "standard" REST API response.

[danieljimeneznz]

The Idea here was to keep the retry strategy decoupled from the HTTP Status. Hence even for HTTP Success we check the retry flag. This change breaks that.

I see. It is a bit confusing when later describing the default according to HTTP status code and no field always means retry=True. Neither this default nor the HTTP status code is configurable so I implemented what I expected normal given a "standard" REST API response.

We could probably add in a configuration for completely ignoring retries when the status code is successful? @kedarkrishnan thoughts?

[kedarkrishnan]

The Idea here was to keep the retry strategy decoupled from the HTTP Status. Hence even for HTTP Success we check the retry flag. This change breaks that.

I see. It is a bit confusing when later describing the default according to HTTP status code and no field always means retry=True. Neither this default nor the HTTP status code is configurable so I implemented what I expected normal given a "standard" REST API response.

We could probably add in a configuration for completely ignoring retries when the status code is successful? @kedarkrishnan thoughts?

@danieljimeneznz That sounds like a good idea, it would not break the existing functionality.