Throw WARN/INFO instead of ERROR on 404

[karoldrazek]

Please kindly consider throwing WARN or INFO rather than ERROR when there's 404 being thrown from repository.

Relevant code part:
https://github.com/Cognifide/knotx/blob/master/knotx-core/src/main/java/io/knotx/repository/http/HttpRepositoryConnectorProxyImpl.java#L151

[toniedzwiedz]

    if (httpResponse.statusCode() >= 300 && httpResponse.statusCode() < 400) { //redirect responses
      LOGGER.info("Repository 3xx response: {}, Headers[{}]", httpResponse.statusCode(),
          DataObjectsUtil.toString(httpResponse.headers()));
    } else if (httpResponse.statusCode() != 200) {
      LOGGER.error("Repository error response: {}, Headers[{}]", httpResponse.statusCode(),
          DataObjectsUtil.toString(httpResponse.headers()));
    }

I'm not sure I understand this correctly but doesn't this effectively treat a number of 2xx family status codes as errors?

As for @karoldrazek 's suggestion, perhaps the log level could be different for 4xx codes (warn/info) and for 5xx codes (error), the rationale being that the 400s can usually be fixed by the client by changing the request accordingly, whereas the 500s are server-side errors beyond the client's control.

[marcinczeczko]

@toniedzwiedz check the condition, the response is logged as error if status code is different than 200.
Anyway, this method is a great candidate for someone who'd like to contribute first time. In my opinion that if statement should log the repository responses in a following way:

• If status code is 2xx log on debug level
• If status code is 3xx log as info
• If status code is 4xx or 5xx log as warn - as it's not an application error but rather something unexpected that should warn the operator

And small tip. Instead of checking ranges of status codes, you can use a HttpStatusClass enum that simplifies response code checking, e.g., in order to test if the 500 =< code < 600 use:

import io.netty.handler.codec.http.HttpStatusClass;

HttpStatusClass.SERVER_ERROR.contains(httpResponse.statusCode());

[michalwronowski]

Logging 5xx on error level as @toniedzwiedz suggested seems very reasonable.

[malaskowski]

Fixed in #427