bharathgunapati commented on PR #35: URL: https://github.com/apache/flink-connector-http/pull/35#issuecomment-6572808449
Hi @featzhang, thank you for this contribution and for the detailed Jira. The motivation makes sense to me: operators do need failures to be visible above DEBUG without enabling full request tracing. A fair amount has landed on main since this was opened, so I wanted to share where things stand before you spend time rebasing: - On the sink side, #54 replaced the status-code checker with a response classifier plus client-level retries, so the `JavaNetSinkHttpClient` hunk here no longer applies. #61 (currently open) builds on that and logs retryable and transport failures at WARN and fatal failures at ERROR, with the original throwable attached. It covers what this PR set out to do for the sink, so it may be simplest to drop the sink changes here. - On the lookup side, I think the remaining gap on main is `continue-on-error=true`, where failures are only logged at DEBUG. This PR keeps DEBUG for that case and raises the severity only when the exception is about to propagate, and at that point Flink already fails the task with a stack trace. So I am not sure it changes visibility where it is currently missing. Would you be open to agreeing on the intended behaviour for tolerated errors first, and then re-scoping the PR to lookup only on top of #61? I am happy to help with that. If it does continue, a few smaller things I noticed: - The PR description and FLINK-39367 mention three options, an `HttpErrorLogger` class and 12 unit tests, but the diff has a single `http.error.log.severity` option and no tests. Could they be brought in line? - The exception is not passed to the logger, so stack traces are lost. Passing it as the last SLF4J argument would fix that. - The existing `logResponse` and `logExceptionResponse` calls are still there, so each failure is now logged twice. - `retryAttempt` is always 0 and `requestBody` is always null at the call sites, so those parameters could probably be dropped. - The new header masking changes the documented MAX behaviour of `http.logging.level`, which was agreed to show clear content for development debugging. It may be worth a separate Jira so it can be discussed on its own. Thanks again, and sorry the ground moved under this one. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
