[GitHub] [pulsar] BewareMyPower commented on pull request #19147: [fix][broker] catch exception for brokerInterceptor
BewareMyPower commented on PR #19147: URL: https://github.com/apache/pulsar/pull/19147#issuecomment-1383085731 @Jason918 It's not related to whether the interceptor developers are core developers of Pulsar. It's related to the fact that there is no clear definition in Pulsar about how the broker handles an exception from the interceptor. I said "complicated" because they have to look deeper into the code when they found the interceptor threw an exception and caused an undefined behavior like this PR and its related issue. -- 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: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #19147: [fix][broker] catch exception for brokerInterceptor
BewareMyPower commented on PR #19147: URL: https://github.com/apache/pulsar/pull/19147#issuecomment-1377017341 @eolivelli @aloyszhang Totally, I agree that we should narrow down the change only to avoid breaking changes. But further more, we might need to start a discussion about how the broker should handle the exception from the interceptor. -- 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: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #19147: [fix][broker] catch exception for brokerInterceptor
BewareMyPower commented on PR #19147: URL: https://github.com/apache/pulsar/pull/19147#issuecomment-1376721824 I also found another reasonable explanation for why do we need to catch the exception in Kafka: https://github.com/apache/kafka/blob/2060b057b055b7e8cc62cbcf8aed710ca8bbe671/clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerInterceptors.java#L63 > // do not propagate interceptor exception, log and continue calling other interceptors If one interceptor throws an exception that is not swallowed, the rest interceptors would be skipped. But there is no way to handle the exception correctly (e.g. determine whether to store the message) except swallowing it. -- 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: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #19147: [fix][broker] catch exception for brokerInterceptor
BewareMyPower commented on PR #19147: URL: https://github.com/apache/pulsar/pull/19147#issuecomment-1376719464 > If I have an interceptor that MUST, for instance, add a metadata header to every Message, and that is not happening due an internal error, we MUST NOT let the Message be stored. Actually the interceptor should not cause any side effect. But there is no way in Java to use a "const" parameter. If we want to allow the modifications, we should let the API return an error code or throw a checked exception to indicate if the modification succeeded. If you allow the existing API to throw an exception and does not caught it, it might lead to an unexpected state at broker. Because the exception will be propagated and the upstream does not know how to handle the error. You can see the original issue (#19146) that the broker does not handle the exception well. It just sends the duplicated ACK responses. Let's talk back to your scenario. Yeah in your case the message should not be stored. But what if the exception was just thrown by an implementation error of the interceptor? e.g. the interceptor try to parse the message value as an integer, but the value might not be an integer. In this case, the message should be stored and the exception that was not processed inside the interceptor should be swallowed by broker. In short, to handle the case you mentioned, the best solution is to add a new API that throws a checked exception, not let the uncaught exception of the interceptor affect the broker. -- 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: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #19147: [fix][broker] catch exception for brokerInterceptor
BewareMyPower commented on PR #19147: URL: https://github.com/apache/pulsar/pull/19147#issuecomment-1374850627 @eolivelli IMO, the interceptor should not be a fail-fast component. `Throwable` is caught for all other similar components currently. In addition to the `ConsumerInterceptor` example I've mentioned, the exception from `MessageListener` is also swallowed. https://github.com/apache/pulsar/blob/badd69b8e012162feb28c86ca9de527cbfd1ac22/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java#L1124-L1125 > But it would be better if BrokerInterceptor maintainer can decide if the exception should be thrown. @Jason918 It could make things more complicated. It's unsafe to give an interceptor the power to terminate the broker. -- 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: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
