Enabling Retry for Meta Data Fetch

[Yash Tailor]

Hi Apache kafka team,

As per the documentation, the kafka producer directly communicates with the
leader of the topic partition to produce a message. In order to fetch the
leader information, it makes a meta data fetch call before its first
produce, further caches it and refreshes depending upon metadata.max.age.ms
 and metadata.max.idle.ms.

During the meta data fetch call, it makes use of partitionsFor() call which
has an upper bound of max.block.ms. What if the broker it tried to connect
to is not reachable, how can we enable the retry of the same call
preferably with another broker (as we have multiple brokers in the
bootstrap servers list)? It looks like the retry configured for the
producer comes into picture when there is a transient error while
receiving the ack back from the broker when the message is sent by the
client (aka the sender thread) and is not applicable for steps that happen
before facilitating it aka fetching the metadata.

Can you help us understand if there is any property which will enable the
said retry or will we need to take care of this situation by ourselves? I
am wondering if properties like connections.max.idle.ms, request.timeout.ms,
rec could play a role in facilitating the retries? And properties like
reconnect.backoff.ms and reconnect.backoff.max.ms can help reconnect with
other brokers when one has already failed?

*Yash Tailor*

[jira] [Resolved] (KAFKA-15713) KRaft support in SaslClientsWithInvalidCredentialsTest

[Chia-Ping Tsai (Jira)]

 [ 
https://issues.apache.org/jira/browse/KAFKA-15713?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Chia-Ping Tsai resolved KAFKA-15713.

Fix Version/s: 3.9.0
   Resolution: Fixed

> KRaft support in SaslClientsWithInvalidCredentialsTest
> --
>
> Key: KAFKA-15713
> URL: https://issues.apache.org/jira/browse/KAFKA-15713
> Project: Kafka
>  Issue Type: Task
>  Components: core
>Reporter: Sameer Tejani
>Assignee: Pavel Pozdeev
>Priority: Minor
>  Labels: kraft, kraft-test, newbie
> Fix For: 3.9.0
>
>
> The following tests in SaslClientsWithInvalidCredentialsTest in 
> core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala
>  need to be updated to support KRaft
> 125 : def testAclCliWithAuthorizer(): Unit = {
> 130 : def testAclCliWithAdminAPI(): Unit = {
> 186 : def testProducerConsumerCliWithAuthorizer(): Unit = {
> 191 : def testProducerConsumerCliWithAdminAPI(): Unit = {
> 197 : def testAclCliWithClientId(): Unit = {
> 236 : def testAclsOnPrefixedResourcesWithAuthorizer(): Unit = {
> 241 : def testAclsOnPrefixedResourcesWithAdminAPI(): Unit = {
> 268 : def testInvalidAuthorizerProperty(): Unit = {
> 276 : def testPatternTypes(): Unit = {
> Scanned 336 lines. Found 0 KRaft tests out of 9 tests



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (KAFKA-17021) Migrate AclCommandTest to new test infra

[Chia-Ping Tsai (Jira)]

Chia-Ping Tsai created KAFKA-17021:
--

 Summary: Migrate AclCommandTest to new test infra
 Key: KAFKA-17021
 URL: https://issues.apache.org/jira/browse/KAFKA-17021
 Project: Kafka
  Issue Type: Improvement
Reporter: Chia-Ping Tsai
Assignee: Chia-Ping Tsai


as title. Also, it would be great to rewrite it by java



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Resolved] (KAFKA-12227) Add method "Producer#send" to return CompletionStage instead of Future

[Chia-Ping Tsai (Jira)]

 [ 
https://issues.apache.org/jira/browse/KAFKA-12227?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Chia-Ping Tsai resolved KAFKA-12227.

Resolution: Duplicate

>  Add method "Producer#send" to return CompletionStage instead of Future
> ---
>
> Key: KAFKA-12227
> URL: https://issues.apache.org/jira/browse/KAFKA-12227
> Project: Kafka
>  Issue Type: New Feature
>Reporter: Chia-Ping Tsai
>Assignee: Chia-Ping Tsai
>Priority: Major
>
> Producer and KafkaProducer return a java.util.concurrent.Future from their 
> send methods. This makes it challenging to write asynchronous non-blocking 
> code given Future's limited interface. Since Kafka now requires Java 8, we 
> now have the option of using CompletionStage and/or CompletableFuture that 
> were introduced to solve this issue. It's worth noting that the Kafka 
> AdminClient solved this issue by using org.apache.kafka.common.KafkaFuture as 
> Java 7 support was still required then.
>  
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-XXX%3A+Return+CompletableFuture+from+KafkaProducer.send



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (KAFKA-17022) Fix error-prone in KafkaApis#handleFetchRequest

[Chia-Ping Tsai (Jira)]

Chia-Ping Tsai created KAFKA-17022:
--

 Summary: Fix error-prone in KafkaApis#handleFetchRequest 
 Key: KAFKA-17022
 URL: https://issues.apache.org/jira/browse/KAFKA-17022
 Project: Kafka
  Issue Type: Improvement
Reporter: Chia-Ping Tsai


 `createResponse`[0] references a variable out of scope, and so that is 
error-prone since it could be not initialized when executing. We should do a 
bit refactor to add `unconvertedFetchResponse` to `createResponse.

[0] 
https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaApis.scala#L939



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (KAFKA-17023) add PCollectionsImmutableMap to ConcurrentMapBenchmark

[Chia-Ping Tsai (Jira)]

Chia-Ping Tsai created KAFKA-17023:
--

 Summary: add PCollectionsImmutableMap to ConcurrentMapBenchmark
 Key: KAFKA-17023
 URL: https://issues.apache.org/jira/browse/KAFKA-17023
 Project: Kafka
  Issue Type: Improvement
Reporter: Chia-Ping Tsai
Assignee: Chia-Ping Tsai


PCollectionsImmutableMap is used in code base, and so we should consider add it 
to benchmark :)



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Re: [DISCUSS] KIP-1027 Add MockFixedKeyProcessorContext

[Shashwat Pandey]

Hi Matthias,

That makes sense to me! I updated the code, definitely want to get your
perspective on whether or not we want to support the
`createFixedKeyRecord(Record)` method, since we already have the
`TestRecord` defined in the utils, it might be cleaner to just support the
`createFixedKeyRecord(TestRecord)` method.

For reference -
https://github.com/s7pandey/kafka/commit/8ac92509d455d8175381a9b4c83900218941bf05#diff-2a3e6e23894a888e8c2fa486e2330f42b8fb28fe2216ba182e27d3d14958457b

Also, looks like I do not have access to update the KIP, my confluence
account is active now (s7pandey) but I think I need some permissions on the
actual KIP page.

Shashwat

On Wed, Jun 12, 2024 at 8:32 PM Matthias J. Sax  wrote:

> I believe the class name was picked on purpose, to make clear that it
> should not be used -- the problem is, that the class is in a public
> package and is by itself public (that's unfortunately require, given how
> Java works).
>
> Of course, it's also in the JavaDocs that the class is internal and
> should not be used, but not everyone reads the JavaDocs necessarily, so
> making it part of the name makes it much more explicit, what I believe
> is a good thing.
>
> I would consider it a fix/improvement, if we could exclude
> `InternalFixedKeyRecordFactory` from JavaDoc generation during the
> release build -- but I don't think we need a KIP for this, as I would
> rather consider it a bug-fix to exclude an internal class in the
> JavaDocs build step.
>
>
> -Matthias
>
> On 6/12/24 4:47 PM, Shashwat Pandey wrote:
> > Hi Matthias,
> >
> > I think that strategy definitely works, abstracting away changes to
> > FixedKeyRecord from users,  I can put that new factory class and update
> the
> > KIP.
> >
> > This might be a discussion for another KIP, but would it also make sense
> to
> > rename the
> > InternalFixedKeyRecordFactory to just FixedKeyRecordFactory also make
> sense?
> >
> >
> > Regards,
> > Shashwat Pandey
> >
> >
> > On Mon, Jun 10, 2024 at 5:07 PM Matthias J. Sax 
> wrote:
> >
> >> Shaswhat,
> >>
> >> any updates on this KIP? -- I still think that recommending to use
> >> `InternalFixedKeyRecordFactory` is not the best way to write test code.
> >> Changing `FixedKeyRecord` constructor (as I mentioned in my last email)
> >> might not be a good solution either.
> >>
> >> Maybe a cleaner way would be (so sidestep this problem), to add a new
> >> public "factory class" into the test package to generate
> >> FixedKeyRecords, and this factory could internally use
> >> `InternalFixedKeyRecordFactory`? It looks cleaner to me from an API POV,
> >> and if we change anything how `FixedKeyRecord` can be created, it would
> >> become a non-user-facing / internal change to the "factory" we provide.
> >>
> >>
> >> -Matthias
> >>
> >> On 5/22/24 12:02 AM, Matthias J. Sax wrote:
> >>> I was not aware of `InternalFixedKeyRecordFactory`. As the name
> >>> indicates, it's considered an internal class, so not sure if we should
> >>> recommend to use it in test...
> >>>
> >>> I understand why this class is required, and why it was put into a
> >>> public package; the way Java works, enforces this. Not sure if we could
> >>> find a better solution.
> >>>
> >>> Might be good to hear from others.
> >>>
> >>>
> >>> -Matthias
> >>>
> >>> On 5/21/24 3:57 PM, Shashwat Pandey wrote:
>  Looking at the ticket and the sample code, I think it would be
>  possible to
>  continue using `InternalFixedKeyRecordFactory` as the avenue to create
>  `FixedKeyRecord`s in tests. As long as there was a
>  MockFixedKeyProcessorContext, I think we would be able to test
>  FixedKeyProcessors without a Topology.
> 
>  I created a sample repo with the `MockFixedKeyProcessorContext` here
> is
>  what I think the tests would look like:
> 
> >>
> https://github.com/s7pandey/kafka-processor-tests/blob/main/src/test/java/com/example/demo/MyFixedKeyProcessorTest.java
> 
> 
> 
>  On Mon, May 20, 2024 at 9:05 PM Matthias J. Sax 
> >> wrote:
> 
> > Had a discussion on
> https://issues.apache.org/jira/browse/KAFKA-15242
> > and it was pointed out, that we also need to do something about
> > `FixedKeyRecord`. It does not have a public constructor (what is
> > correct; it should not have one). However, this makes testing
> > `FixedKeyProcessor` impossible w/o extending `FixedKeyRecord`
> manually
> > what does not seem to be right (too clumsy).
> >
> > It seems, we either need some helper builder method (but not clear to
> >> me
> > where to add it in an elegant way) which would provide us with a
> > `FixedKeyRecord`, or add some sub-class to the test-utils module
> which
> > would extend `FixedKeyRecord`? -- Or maybe an even better solution? I
> > could not think of something else so far.
> >
> >
> > Thoughts?
> >
> >
> > On 5/3/24 9:46 AM, Matthias J. Sax wrote:
> >> Please also update the KIP.
> >>
> 

Jenkins build is unstable: Kafka » Kafka Branch Builder » trunk #3040

[Apache Jenkins Server]

See 

Jenkins build is still unstable: Kafka » Kafka Branch Builder » trunk #3041

[Apache Jenkins Server]

See 

Re: [VOTE] KIP-1022 Formatting and Updating Features

[José Armando García Sancio]

Thanks for the update Colin. The changes make sense to me.

Are you planning to update the KIP to reflect this new RPC version? It
would be good to document the semantics explained above in the KIP.

Thanks!

On Fri, Jun 21, 2024 at 8:22 PM Justine Olshan
 wrote:
>
> Ok makes sense. I will update my PR.
>
> On Fri, Jun 21, 2024 at 5:09 PM Colin McCabe  wrote:
>
> > I think it's better to suppress the response in v3. The issue with
> > modifying it is that there may be scenarios where [1, 1] is the actual
> > supported range, and we'd want to know that. But leaving out the feature
> > should be OK for older clients (it will be the case with clients old enough
> > to send a v0, v1, or v2 ApiVersionsRequest anyway)
> >
> > best,
> > Colin
> >
> > On Fri, Jun 21, 2024, at 16:46, Justine Olshan wrote:
> > > Thanks Colin,
> > >
> > > This makes sense to me. Namely in the case where we perhaps don't want to
> > > support version 0 anymore, we need the range to be able to not include 0.
> > > (In other words, we can't assume 0 is supported)
> > > It is unfortunate that this change is a bit tricky, but I think it's the
> > > best option.
> > >
> > > Can you clarify
> > >> The server will simply leave out the features whose minimum supported
> > > value is 0 for clients that send v3
> > >
> > > For 3.8, I planned to set the 0s in the response to 1. Is it better to
> > > suppress the zero version features in the response so we are consistent
> > > between trunk and 3.8?
> > >
> > > Thanks,
> > > Justine
> > >
> > > On Fri, Jun 21, 2024 at 4:34 PM Colin McCabe  wrote:
> > >
> > >> Hi all,
> > >>
> > >> It seems that there was a bug in older versions of Kafka which caused
> > >> deserialization problems when a supported feature range included 0. For
> > >> example, the range for group.version of [0, 1] would be a problem in
> > this
> > >> situation.
> > >>
> > >> This obviously makes supportedVersions kind of useless. Any feature that
> > >> doesn't exist today is effectively at v0 today (v0 is equivalent to
> > "off").
> > >> But if we can't declare that the server supports [0, 1] or similar, we
> > >> can't declare that it supports the feature being off. Therefore, no
> > rolling
> > >> upgrades are possible.
> > >>
> > >> We noticed this bug during the 3.8 release when we noticed problems in
> > >> upgrade tests. As an addendum to KIP-1022, we're adding the following
> > >> solution:
> > >>
> > >> - There will be a new v4 for ApiVersionsRequest
> > >>
> > >> - Clients that sent v4 will promise to correctly handle ranges that
> > start
> > >> with 0, such as [0, 1]
> > >>
> > >> - The server will simply leave out the features whose minimum supported
> > >> value is 0 for clients that send v3
> > >>
> > >> - ApiVersionsRequest v4 will be supported in AK 3.9 and above. AK 3.8
> > will
> > >> ship with ApiVersionsRequest v3 just as today.
> > >>
> > >> thanks,
> > >> Colin
> > >>
> > >>
> > >> On Mon, Apr 15, 2024, at 11:01, Justine Olshan wrote:
> > >> > Hey folks,
> > >> >
> > >> > Thanks everyone! I will go ahead and call it.
> > >> > The KIP passes with the following +1 votes:
> > >> >
> > >> > - Andrew Schofield (non-binding)
> > >> > - David Jacot (binding)
> > >> > - José Armando García Sancio (binding)
> > >> > - Jun Rao (binding)
> > >> >
> > >> > Thanks again,
> > >> > Justine
> > >> >
> > >> > On Fri, Apr 12, 2024 at 11:16 AM Jun Rao 
> > >> wrote:
> > >> >
> > >> >> Hi, Justine,
> > >> >>
> > >> >> Thanks for the KIP. +1
> > >> >>
> > >> >> Jun
> > >> >>
> > >> >> On Wed, Apr 10, 2024 at 9:13 AM José Armando García Sancio
> > >> >>  wrote:
> > >> >>
> > >> >> > Hi Justine,
> > >> >> >
> > >> >> > +1 (binding)
> > >> >> >
> > >> >> > Thanks for the improvement.
> > >> >> > --
> > >> >> > -José
> > >> >> >
> > >> >>
> > >>
> >



-- 
-José

Re: [DISCUSS] KIP-1027 Add MockFixedKeyProcessorContext

[Matthias J. Sax]

Thanks for the update.

About the wiki account. Creating the account was done by Infra, but 
setting permissions is on us. Fixed.



About the KIP:

Should we pass-in key/value/ts/headers and mimic the constructors for 
`TestRecord` (ie, have similar createXxx(...) overloads as factory 
methods as the constructor overloads) instead of `Record` and 
`TestRecord`? Or is there some benefit I am missing to pass in 
`Record/TestRecord` ?



-Matthias


On 6/22/24 9:32 AM, Shashwat Pandey wrote:

Hi Matthias,

That makes sense to me! I updated the code, definitely want to get your
perspective on whether or not we want to support the
`createFixedKeyRecord(Record)` method, since we already have the
`TestRecord` defined in the utils, it might be cleaner to just support the
`createFixedKeyRecord(TestRecord)` method.

For reference -
https://github.com/s7pandey/kafka/commit/8ac92509d455d8175381a9b4c83900218941bf05#diff-2a3e6e23894a888e8c2fa486e2330f42b8fb28fe2216ba182e27d3d14958457b

Also, looks like I do not have access to update the KIP, my confluence
account is active now (s7pandey) but I think I need some permissions on the
actual KIP page.

Shashwat

On Wed, Jun 12, 2024 at 8:32 PM Matthias J. Sax  wrote:


I believe the class name was picked on purpose, to make clear that it
should not be used -- the problem is, that the class is in a public
package and is by itself public (that's unfortunately require, given how
Java works).

Of course, it's also in the JavaDocs that the class is internal and
should not be used, but not everyone reads the JavaDocs necessarily, so
making it part of the name makes it much more explicit, what I believe
is a good thing.

I would consider it a fix/improvement, if we could exclude
`InternalFixedKeyRecordFactory` from JavaDoc generation during the
release build -- but I don't think we need a KIP for this, as I would
rather consider it a bug-fix to exclude an internal class in the
JavaDocs build step.


-Matthias

On 6/12/24 4:47 PM, Shashwat Pandey wrote:

Hi Matthias,

I think that strategy definitely works, abstracting away changes to
FixedKeyRecord from users,  I can put that new factory class and update

the

KIP.

This might be a discussion for another KIP, but would it also make sense

to

rename the
InternalFixedKeyRecordFactory to just FixedKeyRecordFactory also make

sense?



Regards,
Shashwat Pandey


On Mon, Jun 10, 2024 at 5:07 PM Matthias J. Sax 

wrote:



Shaswhat,

any updates on this KIP? -- I still think that recommending to use
`InternalFixedKeyRecordFactory` is not the best way to write test code.
Changing `FixedKeyRecord` constructor (as I mentioned in my last email)
might not be a good solution either.

Maybe a cleaner way would be (so sidestep this problem), to add a new
public "factory class" into the test package to generate
FixedKeyRecords, and this factory could internally use
`InternalFixedKeyRecordFactory`? It looks cleaner to me from an API POV,
and if we change anything how `FixedKeyRecord` can be created, it would
become a non-user-facing / internal change to the "factory" we provide.


-Matthias

On 5/22/24 12:02 AM, Matthias J. Sax wrote:

I was not aware of `InternalFixedKeyRecordFactory`. As the name
indicates, it's considered an internal class, so not sure if we should
recommend to use it in test...

I understand why this class is required, and why it was put into a
public package; the way Java works, enforces this. Not sure if we could
find a better solution.

Might be good to hear from others.


-Matthias

On 5/21/24 3:57 PM, Shashwat Pandey wrote:

Looking at the ticket and the sample code, I think it would be
possible to
continue using `InternalFixedKeyRecordFactory` as the avenue to create
`FixedKeyRecord`s in tests. As long as there was a
MockFixedKeyProcessorContext, I think we would be able to test
FixedKeyProcessors without a Topology.

I created a sample repo with the `MockFixedKeyProcessorContext` here

is

what I think the tests would look like:




https://github.com/s7pandey/kafka-processor-tests/blob/main/src/test/java/com/example/demo/MyFixedKeyProcessorTest.java




On Mon, May 20, 2024 at 9:05 PM Matthias J. Sax 

wrote:



Had a discussion on

https://issues.apache.org/jira/browse/KAFKA-15242

and it was pointed out, that we also need to do something about
`FixedKeyRecord`. It does not have a public constructor (what is
correct; it should not have one). However, this makes testing
`FixedKeyProcessor` impossible w/o extending `FixedKeyRecord`

manually

what does not seem to be right (too clumsy).

It seems, we either need some helper builder method (but not clear to

me

where to add it in an elegant way) which would provide us with a
`FixedKeyRecord`, or add some sub-class to the test-utils module

which

would extend `FixedKeyRecord`? -- Or maybe an even better solution? I
could not think of something else so far.


Thoughts?


On 5/3/24 9:46 AM, Matthias J. Sax wrote:

Please also update the KIP.

To get a wiki account crea

[jira] [Created] (KAFKA-17024) add integration test for TransactionsCommand

[Chia-Ping Tsai (Jira)]

Chia-Ping Tsai created KAFKA-17024:
--

 Summary: add integration test for TransactionsCommand
 Key: KAFKA-17024
 URL: https://issues.apache.org/jira/browse/KAFKA-17024
 Project: Kafka
  Issue Type: Test
Reporter: Chia-Ping Tsai
Assignee: Chia-Ping Tsai


as title. currently we have only UT for TransactionsCommand



--
This message was sent by Atlassian Jira
(v8.20.10#820010)