Enable HttpPostRequestCallback to fail requests #124

amstee · 2024-09-09T14:58:25Z

Description

Added support for HttpPostRequestCallback to fail requests by throwing a PostRequestCallbackException

PR Checklist

Tests added
Changelog updated

davidradl · 2024-09-10T10:48:36Z

src/main/java/com/getindata/connectors/http/HttpPostRequestCallback.java

@@ -25,5 +25,5 @@ void call(
        RequestT requestEntry,
        String endpointUrl,
        Map<String, String> headerMap
-    );
+    ) throws PostRequestCallbackException;


is there a backwards compatibility issue - as you are changing a method that existing user code could be using?

Hey 👋 , I believe this is backwards compatible as classes still implement the interface even if they don't declare throws PostRequestCallbackException , but am no Java expert.

davidradl · 2024-09-10T10:51:26Z

.../getindata/connectors/http/internal/sink/httpclient/JavaNetSinkHttpClientConnectionTest.java

+
+            assertThat(response.getSuccessfulRequests()).isEmpty();
+            assertThat(response.getFailedRequests()).isNotEmpty();
+        } catch (Exception e) {


do we need the catch? Why not put the checked exceptions on the message then the stack trace will point to the real error.

Removed try-catch

davidradl · 2024-09-10T10:57:10Z

README.md

@@ -375,6 +375,12 @@ and then reference identifier `rest-lookup-logger` in the HTTP lookup DDL proper
 is provided.


+- Callback Errors:
+
+   It is also possible to declare if a request should be considered failed from the [HttpPostRequestCallback](src/main/java/com/getindata/connectors/http/HttpPostRequestCallback.java) by throwing a 


Is there a reason why this is on on request call backs and not responses?

I find the words difficult to parse.
It is also possible to declare if a request should be considered failed from the

Can we say something like :

Throw HttpPostRequestCallbackException to indicate that a request should be considered as failed in your custom callback. The reason you might want to do this is ..... The side effects of doing this are .....

Updated the description, lmk if this is easier to read

davidradl · 2024-09-10T10:58:41Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/JavaNetHttpPollingClient.java

+            );
+        } catch (PostRequestCallbackException e) {
+            log.debug("Error during post request callback.", e);
+            return Optional.empty();


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?

same reason as below, I don't think there's a need to bubble up the exception, just treat this request as failed.

davidradl · 2024-09-10T11:02:33Z

src/main/java/com/getindata/connectors/http/internal/sink/httpclient/JavaNetSinkHttpClient.java

@@ -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);


should this be error?

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 👍

Renamed to FailedRequestException, lmk if this makes more sense 🙏

grzegorz8

Looks good to me.
@kristoffSC Any thoughts?

MarekMaj

Looks good 👍

grzegorz8 · 2024-10-10T09:01:22Z

@amstee Hi! I was about to merge the change but the build failed on checkstyle. Could you please fix that?

amstee · 2024-10-11T15:35:16Z

@amstee Hi! I was about to merge the change but the build failed on checkstyle. Could you please fix that?

@grzegorz8 should be good now 👍

kristoffSC · 2024-10-13T13:46:47Z

Hi @amstee Thanks for your contribution.

TBH I'm not sure if throwing an exception is a best thing to do, I'm not a fan of "communicating via exceptions".
Could you share what is that you want to achieve in your processing and which part of this PR allows you to do it?

Throwing and exceptions is a noticeable cost for JVM and believe me, I have seen apps slowing down significantly when Exceptions were used as a "communication flag". I guess this will happen if we expose it as "request validation" flag for users.

In Readme.md you wrote that:
Currently, the only side effect is to increment the [numRecordsSendErrors counter, as the connector does not support retries yet. However, once retry functionality is implemented, this will allow users to specify if requests should be retried.

I mean, still one can implement its own Callback, throw Runtime exception and "retry" when implemented eventually, will retry it right? No change to API is needed.

I guess you didn't want to introduce an API breaking change and thank you for that.
We can add new method to the callback, marking old one as @deprecated right?

@grzegorz8 @MarekMaj @davidradl
WDYT?

grzegorz8 · 2024-10-16T06:02:22Z

TBH I'm not sure if throwing an exception is a best thing to do, I'm not a fan of "communicating via exceptions".

I agree. I didn't think about that initially.

I mean, still one can implement its own Callback, throw Runtime exception and "retry" when implemented eventually, will retry it right? No change to API is needed.

I guess you didn't want to introduce an API breaking change and thank you for that. We can add new method to the callback, marking old one as @deprecated right?

@grzegorz8 @MarekMaj @davidradl WDYT?

I'm currently working on lookup retries (draft PR: #129). We can think together how to enhance it.

amstee · 2024-10-19T14:00:09Z

Hey 👋 thanks for the review @kristoffSC

Could you share what is that you want to achieve in your processing and which part of this PR allows you to do it?

My goal is to be able to fail some requests through the callback based on something other than HTTP code, body for ex

I mean, still one can implement its own Callback, throw Runtime exception and "retry" when implemented eventually, will retry it right? No change to API is needed.

Wouldn't Runtime exception fail the whole batch though ATM? or do you mean we could catch for that exception instead of having a custom one?

Throwing and exceptions is a noticeable cost for JVM and believe me, I have seen apps slowing down significantly when Exceptions were used as a "communication flag". I guess this will happen if we expose it as "request validation" flag for users.

Hadn't considered that 👍

We can add new method to the callback, marking old one as @deprecated right?

Wouldn't that be a breaking change? are we ok with that?

kristoffSC · 2024-10-25T07:42:45Z

@amstee Thanks for your thoughts.

My goal is to be able to fail some requests through the callback based on something other than HTTP code, body for ex

That's a great use case, lets brain storm on it.
So by fail you mean -> ignore response, retry the request or kill the job (the last one happens whenever there is uncaught exception -> any runtime that was not caught)

Wouldn't Runtime exception fail the whole batch though ATM? or do you mean we could catch for that exception instead of having a custom one?

Yes if this would not be caught by some other code. I was following up to your write down, where you said that "such requests could be retried". For those we don't have to add exception to method's signature, but we could just wrap the callback execution in try/catch and retry in catch block. But that was just me making a point that Exceptions are not good for communication flags, nor for choosing which branch of business code should be executed.

Wouldn't that be a breaking change? are we ok with that?

Yes it would. I was trying to say that we should always think about a way to implement a new feature without introducing breaking changes (which you did and thank you for that) but also, it is worth asking "how this would look like if I could change the API". What are the pros/cons in terms of clean code and clean architecture which are IMO very important in open source projects -> its easier to add new features/bug fixes for new contributors if both, code and architecture are clean.

So we could brain storm about your use case to fail some requests through the callback based on something other than HTTP code.
We can brain storm how we could to add a new method with minimizing user impact. We can introduce a new interface, marking old one as deprecated, we can add new method to old one with default key word or we could just change the API.

@davidradl are you using this callback? Would changing its API have impact on your pipelines?

kristoffSC

IMO we should not use Exception as a communication flag in HttpPostRequestCallback.java

We should think about other way how use case to fail some requests through the callback based on something other than HTTP code, body for ex can be achieved.

I think we can consider API changes.

amstee added 2 commits September 9, 2024 15:52

feat: support marking requests as failed from callback

9e02c27

feat: support marking requests as failed from callback

29139e1

amstee changed the title ~~Postrequest callback exception~~ Enable HttpPostRequestCallback to fail requests Sep 9, 2024

fix: callback exception logs to debug

93ba816

davidradl reviewed Sep 10, 2024

View reviewed changes

amstee added 3 commits September 24, 2024 08:44

renamed exception PostRequestCallbackException to FailedRequestException

084fd52

removed unecessary try-catch

00fdaf6

Improved readme

c32d55b

grzegorz8 approved these changes Oct 2, 2024

View reviewed changes

MarekMaj approved these changes Oct 10, 2024

View reviewed changes

amstee added 3 commits October 11, 2024 16:25

chore: merge main

cb1ed41

Merge branch 'main' into postrequest-callback-exception

ae5f5fe

chore: fix checkstyle

c6f7b49

kristoffSC requested changes Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable HttpPostRequestCallback to fail requests #124

Enable HttpPostRequestCallback to fail requests #124

amstee commented Sep 9, 2024 •

edited

Loading

davidradl Sep 10, 2024

amstee Sep 10, 2024

davidradl Sep 10, 2024

amstee Sep 24, 2024

davidradl Sep 10, 2024 •

edited

Loading

amstee Sep 24, 2024

davidradl Sep 10, 2024 •

edited

Loading

amstee Sep 10, 2024 •

edited

Loading

davidradl Sep 10, 2024

amstee Sep 10, 2024

amstee Sep 24, 2024

grzegorz8 left a comment

MarekMaj left a comment

grzegorz8 commented Oct 10, 2024

amstee commented Oct 11, 2024

kristoffSC commented Oct 13, 2024 •

edited

Loading

grzegorz8 commented Oct 16, 2024

amstee commented Oct 19, 2024

kristoffSC commented Oct 25, 2024 •

edited

Loading

kristoffSC left a comment

Enable HttpPostRequestCallback to fail requests #124

Are you sure you want to change the base?

Enable HttpPostRequestCallback to fail requests #124

Conversation

amstee commented Sep 9, 2024 • edited Loading

Description

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidradl Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidradl Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

amstee Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzegorz8 left a comment

Choose a reason for hiding this comment

MarekMaj left a comment

Choose a reason for hiding this comment

grzegorz8 commented Oct 10, 2024

amstee commented Oct 11, 2024

kristoffSC commented Oct 13, 2024 • edited Loading

grzegorz8 commented Oct 16, 2024

amstee commented Oct 19, 2024

kristoffSC commented Oct 25, 2024 • edited Loading

kristoffSC left a comment

Choose a reason for hiding this comment

amstee commented Sep 9, 2024 •

edited

Loading

davidradl Sep 10, 2024 •

edited

Loading

davidradl Sep 10, 2024 •

edited

Loading

amstee Sep 10, 2024 •

edited

Loading

kristoffSC commented Oct 13, 2024 •

edited

Loading

kristoffSC commented Oct 25, 2024 •

edited

Loading