[broker] change getWorkerService method to throw UnsupportedOperationException #9738
Conversation
…ionException directly.
pulsar-broker/src/test/java/org/apache/pulsar/broker/PulsarServiceTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/PulsarServiceTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
| public WorkerService getWorkerService() throws UnsupportedOperationException { | ||
| return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " + | ||
| "enabled, probably functionsWorkerEnabled is set to false")); |
There was a problem hiding this comment.
I think you can use getWorkerServiceOpt() to check if the workerService is present? Since the workerService might be used in multiple places if we throw an exception here, this means all the places will throw an exception, this is not necessarily the right choice for all operations. So my suggestion is to let the caller to decide the behavior by checking the getWorkerServiceOpt().
There was a problem hiding this comment.
And please check the failed tests, there are 2 Checkstyle violations
There was a problem hiding this comment.
Before submitting the code, I forgot to check the code style. I have already checked and resubmitted
thanks
There was a problem hiding this comment.
Using getWorkerServiceOpt directly may be a more elegant implementation, but it will change too many places, and the expected behavior of most methods is also to throw a clearer exception.
In your opinion, which one is better?
Simplify the implementation or use a more elegant return value.
Maybe you have a better idea, Thank you again for your advice
There was a problem hiding this comment.
In my opinion, If throw exception while getWorkerService(), this might break some other places since it might just want to return a null value. As I said before if all the places are ok for handling the exception that getWorkerService() throws, I'm ok. I just worry about introducing some break changes.
There was a problem hiding this comment.
@codelipenghui I share your opinion.
but if all of the tests are passing, especially integration tests, we should be safe.
There was a problem hiding this comment.
@codelipenghui Thank you for your suggestion. I understand your consideration.
codelipenghui
requested review from
sijie,
315157973,
hangc0276,
jerrypeng,
jiazhai,
merlimat and
srkukarni
…Exception (apache#9738) Fix apache#9633 ### Motivation When we set `functionsWorkerEnabled=false` in `broker.conf`, broker don't support management function of `functionWorker`. But when we try to appy those method. Broker will throw NullPointerException instead of throwing a more user friendly error that explain the problem.
…Exception (#9738) Fix #9633 When we set `functionsWorkerEnabled=false` in `broker.conf`, broker don't support management function of `functionWorker`. But when we try to appy those method. Broker will throw NullPointerException instead of throwing a more user friendly error that explain the problem. (cherry picked from commit baceabd)
codelipenghui
added
cherry-picked/branch-2.7
Fix #9633
Motivation
When we set
functionsWorkerEnabled=falseinbroker.conf, broker don't support management function offunctionWorker.But when we try to appy those method. Broker will throw NullPointerException instead of throwing a more user friendly error that explain the problem.
This PR is trying to fix it.
Modifications
Documentation