Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable HttpPostRequestCallback to fail requests #124

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Enable HttpPostRequestCallback to fail requests #124

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9e02c27
feat: support marking requests as failed from callback
amstee Sep 9, 2024
29139e1
feat: support marking requests as failed from callback
amstee Sep 9, 2024
93ba816
fix: callback exception logs to debug
amstee Sep 9, 2024
084fd52
renamed exception PostRequestCallbackException to FailedRequestException
amstee Sep 24, 2024
00fdaf6
removed unecessary try-catch
amstee Sep 24, 2024
c32d55b
Improved readme
amstee Sep 24, 2024
cb1ed41
chore: merge main
amstee Oct 11, 2024
ae5f5fe
Merge branch 'main' into postrequest-callback-exception
amstee Oct 11, 2024
c6f7b49
chore: fix checkstyle
amstee Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...in/java/com/getindata/connectors/http/internal/sink/httpclient/JavaNetSinkHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private SinkHttpClientResponse prepareSinkHttpClientResponse(
optResponse.orElse(null), sinkRequestEntry, endpointUrl, headerMap);
} catch (PostRequestCallbackException e) {
failedCallback = true;
log.info("request marked as failed due to callback exception", e);
log.debug("request marked as failed due to callback exception", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal behind raising a PostRequestCallbackException is to treat this particular request as failed. Any user who intentionally throws this exception would already be aware of the error from that particular request, so it doesn't feel like there's a need to log higher than debug.

Based on your comments on the PR, I think the naming of that exception class need to be iterated on, will give it a think 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to FailedRequestException, lmk if this makes more sense 🙏

}

// TODO Add response processor here and orchestrate it with statusCodeChecker.
Expand Down
2 changes: 1 addition & 1 deletion ...in/java/com/getindata/connectors/http/internal/table/lookup/JavaNetHttpPollingClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ private Optional<RowData> processHttpResponse(
response, request, "endpoint", Collections.emptyMap()
);
} catch (PostRequestCallbackException e) {
log.warn("Error during post request callback.", e);
log.debug("Error during post request callback.", e);
return Optional.empty();
Copy link
Contributor

@davidradl davidradl Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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?

Copy link
Author

@amstee amstee Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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.

}

Expand Down