Favor unchecked over checked exceptions when sending messages #548
Conversation
|
Thanks for the contribution @jonasgeiregat ! I will review it sometime today. |
There was a problem hiding this comment.
Great work @jonasgeiregat !
A couple of things to address:
-
This is unrelated to your changes but when going through the code I noticed that
Exceptionshould instead be aRuntimeExceptionat https://github.com/spring-projects/spring-pulsar/pull/548/files#diff-f89a2b123104d7393be862fb479fe66a23d2e03cce0efbc338f74c91c2e890ecR243. Do you mind adjusting? If not, I can handle during commit/merge. -
I made a suggested change around keeping the
PulsarClientException.unwrap functionality. -
One area we will need to adjust is the
PulsarOperationswhich still advertises the checked exceptions.
As you pointed out, the PulsarProducerFactory should be adjusted. The fun does not stop there, we also have PulsarConsumerFactory, and PulsarClientFactory. Lmk if you want to handle these as well in a subsequent PR. If you don't have the cycles I am more than happy to pick those up. Or, we could split them up, etc.. Just let me know.
Thanks
spring-pulsar/src/main/java/org/springframework/pulsar/PulsarException.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarTemplate.java
Show resolved
Hide resolved
spring-pulsar/src/test/java/org/springframework/pulsar/core/PulsarTemplateTests.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/test/java/org/springframework/pulsar/core/PulsarTemplateTests.java
Outdated
Show resolved
Hide resolved
...-pulsar/src/test/java/org/springframework/pulsar/reader/PulsarReaderStartMessageIdTests.java
Show resolved
Hide resolved
|
Thanks for the feedback, I'll look into it tomorrow |
There was a problem hiding this comment.
Awesome work @jonasgeiregat ! Thanks for also updating the other factories. I have made some final suggestions but I think after this round we will be good to merge.
Most of the suggestions are the same thing (once in each factory) and they center around the idea that the motivation of the change is to replace the checked PCEX w/ an unchecked version, our new PEX. I don't think we need to catch all when invoking a Pulsar client API, but rather just the PCEX and allow anything unexpected to bubble-up raw as it did before the change.
The one exception is for the PulsarTemplate.doSend because it calls the blocking .get which in turns throws the checked interuppted and execution exceptions and that is why the unwrap call there is helpful.
Again, great work.
spring-pulsar/src/main/java/org/springframework/pulsar/PulsarException.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/DefaultPulsarClientFactory.java
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/DefaultPulsarConsumerFactory.java
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarConsumerFactory.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarOperations.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarOperations.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/DefaultPulsarProducerFactory.java
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarTemplate.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarTemplate.java
Outdated
Show resolved
Hide resolved
|
Processed your comments, sorry for the sloppy commit the other day. |
No need to apologize. I did not find it sloppy. We appreciate your contributions. I will re-review sometime today and get this merged in. |
|
Thanks again for the great contribution! Nice job @jonasgeiregat Closing in favor of 1126c46 |
Should close #547