Re: [DISCUSS] KIP-1006: Remove SecurityManager Support

[Frédérik Rouleau]

Hi all,

When this KIP is intended to be implemented? As KIP-1013 is deprecating
Java 11 in AK 3.7 and removes its support in AK 4.0, maybe the KIP needs an
update.

Regards,

On Mon, Jul 1, 2024 at 10:39 PM Greg Harris 
wrote:

> Hi Mickael,
>
> Thanks for the pointer to that JDK ticket, I did not realize that the
> legacy APIs were going to be degraded instead of removed.
>
> I have updated the KIP to accommodate for this change in the JDK
> implementation. In addition to detecting the removal of the method/classes,
> it will also fall back to the new implementations when encountering an
> UnsupportedOperationException.
> Since this will be a blocker for supporting JDK 23, I'll open a vote thread
> for this next week if I don't get any more comments here.
>
> Thanks,
> Greg
>
> On Wed, Apr 10, 2024 at 10:42 AM Mickael Maison 
> wrote:
>
> > Hi,
> >
> > It looks like some of the SecurityManager APIs are starting to be
> > removed in JDK 23, see
> > - https://bugs.openjdk.org/browse/JDK-8296244
> > - https://github.com/quarkusio/quarkus/issues/39634
> >
> > JDK 23 is currently planned for September 2024.
> > Considering the timelines and that we only drop support for Java
> > versions in major Kafka releases, I think the proposed approach of
> > detecting the APIs to use makes sense.
> >
> > Thanks,
> > Mickael
> >
> > On Tue, Nov 21, 2023 at 8:38 AM Greg Harris
> >  wrote:
> > >
> > > Hey Ashwin,
> > >
> > > Thanks for your question!
> > >
> > > I believe we have only removed support for two Java versions:
> > > 7:
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-118%3A+Drop+Support+for+Java+7
> > > in 2.0
> > > 8:
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=181308223
> > > in 4.0
> > >
> > > In both cases, we changed the gradle sourceCompatibility and
> > > targetCompatibility at the same time, which I believe changes the
> > > "-target" option in javac.
> > >
> > > We have no plans currently for dropping support for 11 or 17, but I
> > > presume they would work in much the same way.
> > >
> > > Hope this helps!
> > > Greg
> > >
> > > On Mon, Nov 20, 2023 at 11:19 PM Ashwin 
> > wrote:
> > > >
> > > > Hi Greg,
> > > >
> > > > Thanks for writing this KIP.
> > > > I agree with you that handling this now will help us react to the
> > > > deprecation of SecurityManager, whenever it happens.
> > > >
> > > > I had a question regarding how we deprecate JDKs supported by Apache
> > Kafka.
> > > > When we drop support for JDK 17, will we set the “-target” option of
> > Javac
> > > > such that the resulting JARs will not load in JVMs which are lesser
> > than or
> > > > equal to that version ?
> > > >
> > > > Thanks,
> > > > Ashwin
> > > >
> > > >
> > > > On Tue, Nov 21, 2023 at 6:18 AM Greg Harris
> > 
> > > > wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > I'd like to invite you all to discuss removing SecurityManager
> > support
> > > > > from Kafka. This affects the client and server SASL mechanism,
> Tiered
> > > > > Storage, and Connect classloading.
> > > > >
> > > > > Find the KIP here:
> > > > >
> > > > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1006%3A+Remove+SecurityManager+Support
> > > > >
> > > > > I think this is a "code higiene" effort that doesn't need to be
> dealt
> > > > > with urgently, but it would prevent a lot of headache later when
> Java
> > > > > does decide to remove support.
> > > > >
> > > > > If you are currently using the SecurityManager with Kafka, I'd
> really
> > > > > appreciate hearing how you're using it, and how you're planning
> > around
> > > > > its removal.
> > > > >
> > > > > Thanks!
> > > > > Greg Harris
> > > > >
> >
>

Permission to contribute to Apache Kafka

[Frédérik Rouleau]

Hi,
As I now have my wiki Id: frouleau and my Jira Id: fred-ro, can I have the
permission to contribute to KIP ?

Regards,

Re: [VOTE] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi,

We can conclude with +4 binding and +3 non-binding votes.
So it's adopted and I will propose it for the 3.8.0 release.
Thank you everyone for your help and participation.

Re: [VOTE] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi all,

I will keep the vote open for a few more hours as I would like to propose
the KIP for the next 3.8.0 release (deadline is the 15th May).
Currently we have +4 binding and +3 non-binding.
Thanks,

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi Sophie,

I have updated the KIP and the PR.

Regards,

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi Sophie,

I agree that the subclasses have limited value and I am not a fan of
"instance of" usage either.
I do not see any problem with adding a field but I would rather name it
something like exceptionOrigin. Any thoughts?

About byteBuffer vs byte[], byteBuffer are more generic and with proper
doc/example, I do not think it's an issue. I will then remove the byte[]
returning methods.

Thanks,


[image: Confluent] <https://www.confluent.io>
Frederik Rouleau
Sr Customer Success Technical Architect
Follow us: [image: Blog]
<https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog>[image:
Twitter] <https://twitter.com/ConfluentInc>[image: LinkedIn]
<https://www.linkedin.com/company/confluent/>[image: Slack]
<https://slackpass.io/confluentcommunity>[image: YouTube]
<https://youtube.com/confluent>

[image: Try Confluent Cloud for Free]
<https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic>


On Tue, Apr 30, 2024 at 10:54 PM Sophie Blee-Goldman 
wrote:

> Actually one more thing, after thinking a bit about how this would be used
> in practice, I'm inclined to agree that maybe distinguishing between key vs
> value errors via subclasses is not the cleanest way to present the API.
> Users would still essentially want to catch the general
> RecordDeserializationException error since in practice, much of the
> handling is likely to be the same between key and value errors. So then to
> distinguish between these, they would end up doing an "instance of" check
> on the exception type. Which feels like an awkward way to answer a question
> that could have just been a simple API on the
> RecordDeserializationException itself. What do you think about getting rid
> of the subclasses and instead adding one more API to the
> RecordDeserializationException that indicates whether it was a key or value
> error?
>
> This could return a simple boolean and be called #isKeyError or something,
> but that feels kind of awkward too. Maybe a better alternative would be
> something like this:
>
> class RecordDeserializationException {
> enum DeserializationExceptionType {
> KEY,
> VALUE
> }
>
>   public DeserializationExceptionType exceptionType();
> }
>
> I also worry that people don't always check for exception subtypes and
> would easily miss the existence of the KeyDeserializationException and
> ValueDeserializationException. Simply adding an API to the
> RecordDeserializationException will make it much easier for users to notice
> and react accordingly, if they care to do something different based on
> whether the error happened during key or value deserialization.
>
> Thoughts?
>
> On Tue, Apr 30, 2024 at 1:45 PM Sophie Blee-Goldman  >
> wrote:
>
> > Hey Fred, I think this is looking good but I just want to follow up on
> > what Kirk asked earlier about having both the ByteBuffer and byte[]
> forms.
> > Can't users just use the ByteBuffer versions and convert them into a
> byte[]
> > themselves? In some cases it maybe makes sense to offer some additional
> > APIs if there is complex processing involved in converting between
> returned
> > types, but ByteBuffer --> byte[] seems pretty straightforward to me :)
> >
> > Generally speaking we try to keep the APIs as tight as possible and offer
> > only what is necessary, and I'd rather leave off "syntactic sugar" type
> > APIs unless there is a clear need. Put another way: it's easy to add
> > additional methods if someone wants them, but it's much harder to remote
> > methods since we have to go through a deprecation cycle. So I'd prefer to
> > just keep only the ByteBuffer versions (or only the byte[] -- don't
> > personally care which of the two)
> >
> > One more small nit: since we're deprecating the old exception
> constructor,
> > can you list that in the "Compatibility, Deprecation, and Migration Plan"
> > section?
> >
> >
> >
> > On Wed, Apr 24, 2024 at 5:35 AM Frédérik Rouleau
> >  wrote:
> >
> >> Hi,
> >>
> >> I have updated the KIP now and the latest version of PR is available.
> >>
> >> About Kirk's questions:
> >>
> >> K11: Yes, both can have a deserialization exception but we deserialize
> the
> >> key first, so if an error occurs then, we do not try to deserialize the
> >> value. So the exception raised is always for key or value.
> >>
> >> K12: Not sure of concrete usage for now, just a sugar feature. I suppose
> >> we
> >> can imagine some use case where you need/want only the first bytes and
> do
> >> not want to waste memory allocating the whole payload (SchemaRegistry's
> >> schema Id or something similar).
> >>
> >> K13: The old constructor is not needed anymore. It is just for
> >> compatibility until removed in a major version. As public we might have
> >> some users using it even if I cannot see any valid reason for this.
> >>
> >> Thanks,
> >> Fred
> >>
> >
>

[VOTE] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi all,

As there is no more activity for a while on the discuss thread, I think we
can start a vote.
The KIP is available on
https://cwiki.apache.org/confluence/display/KAFKA/KIP-1036%3A+Extend+RecordDeserializationException+exception


If you have some feedback or suggestions, please participate to the
discussion thread:
https://lists.apache.org/thread/or85okygtfywvnsfd37kwykkq5jq7fy5

Best regards,
Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi,

I have updated the KIP now and the latest version of PR is available.

About Kirk's questions:

K11: Yes, both can have a deserialization exception but we deserialize the
key first, so if an error occurs then, we do not try to deserialize the
value. So the exception raised is always for key or value.

K12: Not sure of concrete usage for now, just a sugar feature. I suppose we
can imagine some use case where you need/want only the first bytes and do
not want to waste memory allocating the whole payload (SchemaRegistry's
schema Id or something similar).

K13: The old constructor is not needed anymore. It is just for
compatibility until removed in a major version. As public we might have
some users using it even if I cannot see any valid reason for this.

Thanks,
Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi Andrew,

A1. I will change the order of arguments to match.
A2 and A3, Yes the KIP is not updated yet as I do not have a wiki account.
So I must rely on some help to do those changes, which add some delay. I
will try to find someone available ASAP.
A4. I had the same thought. Using keyBuffer and valueBuffer for the
constructor seems fine for me. If no one disagrees, I will do that change.

Thanks,
Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi everyone,

Thanks for all that valuable feedback.
So we have a consensus not to use Record.

I have updated to PR by creating 2 childs classes
KeyDeserializationException and ValueDeserializationException. Those
classes directly embed the required fields. I do not think a wrapper object
would be useful in that case.
I still had to update checkstyle as Headers class is not allowed for import
in the Errors package. I do not think it's an issue to add that
authorization as Headers is already used in consumerRecord, so already
public.

The proposed PR https://github.com/apache/kafka/pull/15691/files

If it's ok I will update the KIP.

Regards,
Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi,

But I guess my main question is really about what metadata we really
> want to add to `RecordDeserializationException`? `Record` expose all
> kind of internal (serialization) metadata like `keySize()`,
> `valueSize()` and many more. For the DLQ use-case it seems we don't
> really want any of these? So I am wondering if just adding
> key/value/ts/headers would be sufficient?
>

I think that key/value/ts/headers, topicPartition and offset are all we
need. I do not see any usage for other metadata. If someone has a use case,
I would like to know it.

So in that case we can directly add the data into the exception. We can
keep ByteBuffer for the local field instead of byte[], that will avoid
memory allocation if users do not require it.
I wonder if we should return the ByteBuffer or directly the byte[] (or both
?) which is more convenient for end users. Any thoughts?
Then we can have something like:

public RecordDeserializationException(TopicPartition partition,
 long offset,
 ByteBuffer key,
 ByteBuffer value,
 Header[] headers,
 long timestamp,
 String message,
 Throwable cause);

public TopicPartition topicPartition();

public long offset();

public long timestamp();

public byte[] key(); // Will allocate the array on call

public byte[] value(); // Will allocate the array on call

public Header[] headers();



Regards,
Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Thanks Sophie,

I can write something in the KIP on how KStreams solves that issue, but as
I can't create a Wiki account, I will have to find someone to do this on my
behalf (if someone can work on solving that wiki account creation, it would
be great).

The biggest difference between Record and ConsumerRecord is that data are
stored respectively using ByteBuffer and Byte array.

For the Record option, the object already exists in the parsing method, so
it's roughly just a parameter type change in the Exception. The point is
just about exposing the Record class externally. By the way, the name
Record is also making some IDE a bit crazy by confusing it with the new
Java Record feature. An alternative could be to create another wrapper type
of just include key and value ByteBuffer in the
RecordDeserializationException itself.

For the ConsumerRecord option, it requires to allocate Byte arrays, even if
the user does not need it (skip the poison pill for example). This might
have some extra cost on GC for some specific use case.

Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi Almog,

I think you do not understand the behavior that was introduced with the
KIP-334.
When you have a DeserializationException, if you set the proper seek call
to skip the faulty record, the next poll call will return the remaining
records to process and not a new list of records. When the KIP was
released, I made a demo project
https://github.com/fred-ro/kafka-poison-pills not sure it's still working,
I should spend time maintaining it. The only issue with the initial KIP is
that you do not have access to the data of the faulty record which makes
DLQ implementation quite difficult.

Regards,
Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi everyone,

I have made some changes to take in account comments. I have replaced the
ConsumerRecord by Record. As it was not allowed by checkstyle, I have
modified its configuration. I hope that's ok.
I find this new version better. Thanks for your help.

Regards,
Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi Kirk,

I have made the test and I confirm that checkstyle is complaining
(Disallowed import - org.apache.kafka.common.record.Record.) if I use
Record class in the RecordDeserialisationException.
An alternative might be to add key(), value(), headers() etc methods
directly in the exception.

Regards,
Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Thanks for your feedback.

Kirk, that's a good point. I will check if there are other ways of raising
an exception than the deserialisation itself.
About Record, I agree I think it would be a better choice and my initial
version was using it. But then I realised that this class might not be
exposed, at least I had some errors from checkstyle. That solution would
also improve GC pressure if you do not use the record by avoiding the
allocation of useless byte arrays.
Maybe someone can confirm that there is no issue by using the Record class.
Matthias, thanks for your comment. Unfortunately I will have to find
someone to do the changes for me as I was not able to create an account on
the wiki.

Regards,
Fred

[DISCUSS] KIP-1036: Extend RecordDeserializationException exception

[Frédérik Rouleau]

Hi everyone,

To make implementation of DLQ in consumer easier, I would like to add the
raw ConsumerRecord into the RecordDeserializationException.

Details are in KIP-1036

.

Thanks for your feedback.

Regards,
Fred