Sounds like we're more or less in agreement here. I think the KIP just
needs one small update still, which is the SerializationExceptionOrigin
enum.
As discussed there are a few options for this and we're all happy to defer
to the preference of the KIP authors, but if we keep the KIP as-is with the
two separate methods in the ProductionExceptionHandler, then imo it makes
the most sense to add the SerializationExceptionOrigin enum to the
ProductionExceptionHandler interface and then add an "origin" parameter to
the new #handleSerializationException method. However you decide to do it,
I'm personally happy to vote on this KIP once the KIP is updated.
Just FYI the 3.8 KIP freeze is this upcoming Wednesday, so if you guys
would like to target 3.8 for this feature, just make sure to update the KIP
and kick off a [VOTE] thread by EOD Monday so that you can close the vote
by EOD Wednesday (since it has to be open for 72 hours).
Thanks again for this sorely needed feature!
On Fri, May 10, 2024 at 10:06 AM Bill Bejeck <bbej...@gmail.com> wrote:
> Great KIP discussion so far by everyone.
> At this point, I'm in agreement with the direction and current state of the
> KIP.
>
> As for having two separate callbacks for the ProductionExceptionHandler,
> I'm somewhat split in that I agree with points raised by Sophie and
> Matthias with my final
> position being to maintain both callbacks. IMHO, while there are several
> things that could go wrong with producing a message, it seems that
> serialization exceptions would be the most common, although I don't have
> any data to back that opinion up. But having said that, should the KIP
> authors decide otherwise, I would be fine with that approach as well.
>
> I'm at the point where I'm comfortable voting for this KIP.
>
> Thanks,
> Bill
>
> On Thu, May 9, 2024 at 4:28 PM Sophie Blee-Goldman <sop...@responsive.dev>
> wrote:
>
> > The type safety issue is definitely not solved by having two separate
> > callbacks. I just think it gets a bit worse by mashing them into one
> > method. At least in the plain #handle method you can be sure that the
> type
> > is ProducerRecord<byte[], byte[]> and in #handleSerialization the type is
> > some POJO.
> >
> > And in theory you can just embed the mapping of sink topics to type/Serde
> > based on your topology. Or let's say your output record keys & values are
> > all Strings, and you want to print the String representation in your
> > handler, rather than the bytes.
> > Having a separate callback means knowing you can simply print the
> > ProducerRecord's key/value in the #handleSerialization method, and will
> > have to use a StringDeserializer to convert the key/value to its String
> > form to print it in the #handle method.
> >
> > Again, I just feel this will be more straightforward and easy for users
> to
> > use correctly, but am satisfied either way. I'll shut up now and wait for
> > the KIP authors to make a call on this one way or another, and then I'm
> > happy to cast my vote
> >
> > On Thu, May 9, 2024 at 10:15 AM Matthias J. Sax <mj...@apache.org>
> wrote:
> >
> > > Thanks Sophie! Makes it much clearer where you are coming from.
> > >
> > > About the Type unsafety: isn't this also an issue for the
> > > `handleSerialziationException` case, because the handler is used for
> all
> > > sink topics, and thus key/value types are not really know w/o taking
> the
> > > sink topic into account? -- So I am not sure if having two handler
> > > methods really helps much with regard to type safety?
> > >
> > > Just want to make this small comment for completeness. Let's hear what
> > > others think. Given that we both don't have a strong opinion but just a
> > > personal preference, we should be able to come to a conclusion quickly
> > > and get this KIP approved for 3.8 :)
> > >
> > >
> > > -Matthias
> > >
> > > On 5/8/24 3:12 PM, Sophie Blee-Goldman wrote:
> > > > Well I definitely don't feel super strongly about it, and more
> > > importantly,
> > > > I'm not a user. So I will happily defer to the preference of anyone
> who
> > > > will actually be using this feature. But I'll explain my reasoning:
> > > >
> > > > There *is* a relevant distinction between these two callbacks --
> > because
> > > > the passed-in record will have a different type depending on whether
> it
> > > was
> > > > a serialization exception or something else. Even if we combined them
> > > into
> > > > a single #handle method, users will still end up implementing two
> > > distinct
> > > > branches depending on whether it was a serialization exception or
> not,
> > > > since that determines the type of the ProducerRecord passed in.
> > > >
> > > > Not to mention they'll need to cast it to a ProducerRecord<byte[],
> > > byte[]>
> > > > when we could have just passed it in as this type via a dedicated
> > > callback.
> > > > And note that because of the generics, they can't do an instanceof
> > check
> > > to
> > > > make sure that the record type is ProducerRecord<byte[], byte[]> and
> > will
> > > > have to suppress the "unchecked cast" warning.
> > > >
> > > > So if we combined the two callbacks, their handler will look
> something
> > > like
> > > > this:
> > > >
> > > > @SuppressWarnings("unchecked")
> > > > public ProductionExceptionHandlerResponse handle(final
> > > ErrorHandlerContext
> > > > context,
> > > > final ProducerRecord<?, ?> record,
> > > > final Exception exception) {
> > > > if (exception instanceof SerializationException) {
> > > > if (exception.origin().equals(KEY)) {
> > > > log.error("Failed to serialize key", exception);
> > > > } else {
> > > > log.error("Failed to serialize value", exception);
> > > > }
> > > >
> > > > } else {
> > > > final ProducerRecord<byte[], byte[]> serializedRecord =
> > > (ProducerRecord<byte[],
> > > > byte[]>) record;
> > > > log.error("Failed to produce record with serialized key={} and
> > serialized
> > > > value={}",
> > > > serializedRecord.key(), serializedRecord.value());
> > > > }
> > > > return ProductionExceptionHandlerResponse.FAIL;
> > > > }
> > > >
> > > > That seems like the most basic case, and it still haswith distinct
> > logic
> > > > even if they ultimately handle exceptions the same way. And looking
> > > forward
> > > > to KIP-1034: Dead-letter queues, it seems all the more likely that
> the
> > > > actual handling response might be different depending on whether
> it's a
> > > > serialization exception or not: a serialized record can probably be
> > > > retried/sent to a DLQ, whereas a record that can't be serialized
> should
> > > not
> > > > (can't, really) be forwarded to a DLQ. So if they're going to have
> > > > completely different implementations depending on whether it's a
> > > > serialization exception, why not just give them two separate
> callbacks?
> > > >
> > > > And that's all assuming the user is perfectly aware of the different
> > > > exception types and their implications for the type of the
> > > ProducerRecord.
> > > > Many people might just miss the existence of the
> > > > RecordSerializationException altogether --
> > > > there are already so many different exception types, ESPECIALLY when
> it
> > > > comes to the Producer. Not to mention they'll need to understand the
> > > > nuances of how the ProducerRecord type changes depending on the type
> of
> > > > exception that's passed in. And on top of all that, they'll need to
> > know
> > > > that there is metadata stored in the RecordSerializationException
> > > regarding
> > > > the origin of the error. Whereas if we just passed in the
> > > > SerializationExceptionOrigin to a #handlerSerialization callback,
> well,
> > > > that's pretty impossible to miss.
> > > >
> > > > That all just seems like a lot for most people to have to understand
> to
> > > > implement a ProductionExceptionHandler, which imo is not at all an
> > > advanced
> > > > feature and should be as straightforward and easy to use as possible.
> > > >
> > > > Lastly -- I don't think it's quite fair to compare this to the
> > > > RecordDeserializationException. We have a dedicated handler that's
> just
> > > for
> > > > deserialization exceptions specifically, hence there's no worry about
> > > users
> > > > having to be aware of the different exception types they might have
> to
> > > deal
> > > > with in the DeserializtionExceptionHandler. Whereas serialization
> > > > exceptions are just a subset of what might get passed in to the
> > > > ProductionExceptionHandler...
> > > >
> > > > Just explaining my reasoning -- in the end I leave it up to the KIP
> > > authors
> > > > and anyone who will actually be using this feature in their
> > applications
> > > :)
> > > >
> > > >
> > > >
> > > > On Tue, May 7, 2024 at 8:35 PM Matthias J. Sax <mj...@apache.org>
> > wrote:
> > > >
> > > >> @Loic, yes, what you describe is exactly what I had in mind.
> > > >>
> > > >>
> > > >>
> > > >> @Sophie, can you elaborate a little bit?
> > > >>
> > > >>> First of all, I agree that it makes sense to maintain the two
> > separate
> > > >>> callbacks for the ProductionExceptionHandler, since one of them is
> > > >>> specifically for serialization exceptions while the other is used
> for
> > > >>> everything/anything else.
> > > >>
> > > >> What makes a serialization exception special compare to other errors
> > > >> that it's valuable to treat it differently? Why can we put
> "everything
> > > >> else" into a single bucket? By your train of though, should we not
> > split
> > > >> out the "everything else" bucket into a different callback method
> for
> > > >> every different error? If no, why not, but only for serialization
> > > errors?
> > > >>
> > > >> From what I believe to remember, historically, we added the
> > > >> ProductionExceptionHandler, and kinda just missed the serialization
> > > >> error case. And later, when we extended the handler we just could
> not
> > > >> re-use the existing callback as it was typed with `<byte[], byte[]>`
> > and
> > > >> it would have been an incompatible change; so it was rather a
> > workaround
> > > >> to add the second method to then handler, but not really intended
> > > design?
> > > >>
> > > >>
> > > >> It's of course only my personal opinion that I believe a single
> > callback
> > > >> method is simpler/cleaner compared to sticking with two, and adding
> > the
> > > >> new exception type to make it backward compatible seems worth it. It
> > > >> also kinda introduces the same patter we use elsewhere (cf KIP-1036)
> > > >> what I actually think is an argument for introducing
> > > >> `RercordSerializationExcetpion`, to unify user experience across the
> > > board.
> > > >>
> > > >> Would be great to hear from others about this point. It's not that I
> > > >> strongly object to having two methods, and I would not block this
> KIP
> > on
> > > >> this question.
> > > >>
> > > >>
> > > >>
> > > >> -Matthias
> > > >>
> > > >>
> > > >> On 5/7/24 3:40 PM, Sophie Blee-Goldman wrote:
> > > >>> First of all, I agree that it makes sense to maintain the two
> > separate
> > > >>> callbacks for the ProductionExceptionHandler, since one of them is
> > > >>> specifically for serialization exceptions while the other is used
> for
> > > >>> everything/anything else. I also think we can take advantage of
> this
> > > fact
> > > >>> to simplify things a bit and cut down on the amount of new stuff
> > added
> > > to
> > > >>> the API by just adding a parameter to the
> > #handleSerializationException
> > > >>> callback and use that to pass in the SerializationExceptionOrigin
> > enum
> > > to
> > > >>> distinguish between key vs value. This way we wouldn't need to
> > > introduce
> > > >>> yet another exception type (the RecordSerializationException) just
> to
> > > >> pass
> > > >>> in this information.
> > > >>>
> > > >>> Thoughts?
> > > >>>
> > > >>> On Tue, May 7, 2024 at 8:33 AM Loic Greffier <
> > > loic.greff...@michelin.com
> > > >>>
> > > >>> wrote:
> > > >>>
> > > >>>> Hi Matthias,
> > > >>>>
> > > >>>> To sum up with the ProductionExceptionHandler callback methods
> (106)
> > > >>>> proposed changes.
> > > >>>>
> > > >>>> A new method ProductionExceptionHandler#handle is added with the
> > > >> following
> > > >>>> signature:
> > > >>>>
> > > >>>>> ProductionExceptionHandlerResponse handle(final
> ErrorHandlerContext
> > > >>>> context, final ProducerRecord<?,?> record, final Exception
> > exception);
> > > >>>>
> > > >>>> The ProducerRecord<?,?> parameter has changed to accept a
> serialized
> > > or
> > > >>>> non-serialized record.
> > > >>>> Thus, the new ProductionExceptionHandler#handle method can handle
> > both
> > > >>>> production exception and serialization exception.
> > > >>>>
> > > >>>> Both old ProductionExceptionHandler#handle and
> > > >>>> ProductionExceptionHandler#handleSerializationException methods
> are
> > > now
> > > >>>> deprecated.
> > > >>>> The old ProductionExceptionHandler#handle method gets a default
> > > >>>> implementation, so users do not have to implement a deprecated
> > method.
> > > >>>>
> > > >>>> To handle backward compatibility, the new
> > > >>>> ProductionExceptionHandler#handle method gets a default
> > > implementation.
> > > >>>>
> > > >>>>> default ProductionExceptionHandlerResponse handle(final
> > > >>>> ErrorHandlerContext context, final ProducerRecord<?, ?> record,
> > final
> > > >>>> Exception exception) {
> > > >>>>> if (exception instanceof RecordSerializationException) {
> > > >>>>> this.handleSerializationException(record,
> > > exception.getCause());
> > > >>>>> }
> > > >>>>>
> > > >>>>> return handle((ProducerRecord<byte[], byte[]>) record,
> > > exception);
> > > >>>>> }
> > > >>>>
> > > >>>> The default implementation either invokes
> > > #handleSerializationException
> > > >> or
> > > >>>> #handle depending on the type of the exception, thus users still
> > > >> relying on
> > > >>>> deprecated ProductionExceptionHandler#handle
> > > >>>> or ProductionExceptionHandler#handleSerializationException custom
> > > >>>> implementations won't break.
> > > >>>>
> > > >>>> The new ProductionExceptionHandler#handle method is now invoked in
> > > case
> > > >> of
> > > >>>> serialization exception:
> > > >>>>
> > > >>>>> public <K, V> void send(final String topic, final K key, final V
> > > value,
> > > >>>> ...) {
> > > >>>>> try {
> > > >>>>> keyBytes = keySerializer.serialize(topic, headers,
> key);
> > > >>>>> ...
> > > >>>>> } catch (final ClassCastException exception) {
> > > >>>>> ...
> > > >>>>> } catch (final Exception exception) {
> > > >>>>>
> > > >>>>> try {
> > > >>>>> response =
> productionExceptionHandler.handle(context,
> > > >>>> record, new
> > > >> RecordSerializationException(SerializationExceptionOrigin.KEY,
> > > >>>> exception));
> > > >>>>> } catch (final Exception e) {
> > > >>>>> ...
> > > >>>>> }
> > > >>>>> }
> > > >>>>> }
> > > >>>>
> > > >>>> To wrap the origin serialization exception and determine whether
> it
> > > >> comes
> > > >>>> from the key or the value, a new exception class is created:
> > > >>>>
> > > >>>>> public class RecordSerializationException extends
> > > >> SerializationException
> > > >>>> {
> > > >>>>> public enum SerializationExceptionOrigin {
> > > >>>>> KEY,
> > > >>>>> VALUE
> > > >>>>> }
> > > >>>>>
> > > >>>>> public
> > > RecordSerializationException(SerializationExceptionOrigin
> > > >>>> origin, final Throwable cause);
> > > >>>>> }
> > > >>>>
> > > >>>> Hope it all makes sense,
> > > >>>> Loïc
> > > >>>>
> > > >>>
> > > >>
> > > >
> > >
> >
>
