date:20240328

See

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

See

Jenkins build is still unstable: Kafka » Kafka Branch Builder » 3.5 #106

See

Re: [DISCUSS] KIP-853: KRaft Controller Membership Changes

2024-03-28 Thread José Armando García Sancio

Jun, thanks a lot for your help. I feel that the KIP is much better
after your detailed input.

If there is no more feedback, I'll start a voting thread tomorrow
morning. I'll monitor KIP-1022's discussion thread and update this KIP
with anything that affects the KIP's specification.

Thanks,
-- 
-José

Re: [DISCUSS] KIP-853: KRaft Controller Membership Changes

Hi, Jose,

Thanks for the explanation. Other than depending on KIP-1022 to be
approved, the KIP looks good to me now.

Jun

On Thu, Mar 28, 2024 at 2:56 PM José Armando García Sancio
 wrote:

> Hi Jun,
>
> See my comments below.
>
> On Thu, Mar 28, 2024 at 11:09 AM Jun Rao  wrote:
> > If I am adding a new voter and it takes a long time (because the new
> voter
> > is catching up), I'd want to know if the request is indeed being
> processed.
> > I thought that's the usage of uncommitted-voter-change.
>
> They can get related information by using the 'kafka-metadata describe
> --replication" command (or the log-end-offset metric from KIP-595).
> That command (and metric) displays the LEO of all of the replicas
> (voters and observers), according to the leader. They can use that
> output to discover if the observer they are trying to add is lagging
> or is not replicating at all.
>
> When the user runs the command above, they don't know the exact offset
> that the new controller needs to reach but they can do some rough
> estimation of how far behind it is. What do you think? Is this good
> enough?
>
> > Also, I am still not sure about having multiple brokers reporting the
> same
> > metric. For example, if they don't report the same value (e.g. because
> one
> > broker is catching up), how does a user know which value is correct?
>
> They are all correct according to the local view. Here are two
> examples of monitors that the user can write:
>
> 1. Is there a voter that I need to remove from the quorum? They can
> create a monitor that fires, if the number-of-offline-voters metric
> has been greater than 0 for the past hour.
> 2. Is there a cluster that doesn't have 3 voters? They can create a
> monitor that fires, if any replica doesn't report three for
> number-of-voters for the past hour.
>
> Is there a specific metric that you have in mind that should only be
> reported by the KRaft leader?
>
> Thanks,
> --
> -José
>

[jira] [Resolved] (KAFKA-16286) KRaft doesn't always notify listener of latest leader

2024-03-28 Thread Jira



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

José Armando García Sancio resolved KAFKA-16286.

Resolution: Fixed

> KRaft doesn't always notify listener of latest leader
> -
>
> Key: KAFKA-16286
> URL: https://issues.apache.org/jira/browse/KAFKA-16286
> Project: Kafka
>  Issue Type: Bug
>  Components: kraft
>Reporter: José Armando García Sancio
>Assignee: José Armando García Sancio
>Priority: Major
> Fix For: 3.8.0
>
>
> If a listener registers with RaftClient after the KRaft replica has 
> transition to follower it will not get notified of the current leader until 
> it has transitioned to another state.
> In a stable cluster the listeners that are not the leader (inactive 
> controllers and brokers) will only get notified when then leader changes.



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

Re: [DISCUSS] KIP-853: KRaft Controller Membership Changes

2024-03-28 Thread José Armando García Sancio

Hi Jun,

See my comments below.

On Thu, Mar 28, 2024 at 11:09 AM Jun Rao  wrote:
> If I am adding a new voter and it takes a long time (because the new voter
> is catching up), I'd want to know if the request is indeed being processed.
> I thought that's the usage of uncommitted-voter-change.

They can get related information by using the 'kafka-metadata describe
--replication" command (or the log-end-offset metric from KIP-595).
That command (and metric) displays the LEO of all of the replicas
(voters and observers), according to the leader. They can use that
output to discover if the observer they are trying to add is lagging
or is not replicating at all.

When the user runs the command above, they don't know the exact offset
that the new controller needs to reach but they can do some rough
estimation of how far behind it is. What do you think? Is this good
enough?

> Also, I am still not sure about having multiple brokers reporting the same
> metric. For example, if they don't report the same value (e.g. because one
> broker is catching up), how does a user know which value is correct?

They are all correct according to the local view. Here are two
examples of monitors that the user can write:

1. Is there a voter that I need to remove from the quorum? They can
create a monitor that fires, if the number-of-offline-voters metric
has been greater than 0 for the past hour.
2. Is there a cluster that doesn't have 3 voters? They can create a
monitor that fires, if any replica doesn't report three for
number-of-voters for the past hour.

Is there a specific metric that you have in mind that should only be
reported by the KRaft leader?

Thanks,
-- 
-José

Re: [DISCUSS] KIP-890 Server Side Defense

2024-03-28 Thread Justine Olshan

Hi there -- another update!

When looking into the implementation for the safe epoch bumps I realized
that we are already populating previousProducerID in memory as part of
KIP-360.
If we are to start using flexible fields, it is better to always use this
information and have an explicit (tagged) field to indicate whether the
client supports KIP-890 part 2.

I've included the extra field and how it is set in the KIP. I've also
updated the KIP to explain that we will be setting the tagged fields when
they are available for all transitions.

Finally, I added clearer text about the transaction protocol versions
included with this KIP. 1 for flexible transaction state records and 2 for
KIP-890 part 2 enablement.

Justine

On Mon, Mar 18, 2024 at 6:39 PM Justine Olshan  wrote:

> Hey there -- small update to the KIP,
>
> The KIP mentioned introducing ABORTABLE_ERROR and bumping TxnOffsetCommit
> and Produce requests. I've changed the name in the KIP to
> ABORTABLE_TRANSACTION and the corresponding exception
> AbortableTransactionException to match the pattern we had for other errors.
> I also mentioned bumping all 6 transactional APIs so we can future
> proof/support the error on the client going forward. If a future change
> wants to have an error scenario that requires us to abort the transaction,
> we can rely on the 3.8+ clients to support it. We ran into issues finding
> good/generic error codes that older clients could support while working on
> this KIP, so this should help in the future.
>
> The features discussion is still ongoing in KIP-1022. Will update again
> here when that concludes.
>
> Justine
>
> On Tue, Feb 6, 2024 at 8:39 AM Justine Olshan 
> wrote:
>
>> I don't think AddPartitions is a good example since we currenly don't
>> gate the version on TV or MV. (We only set a different flag depending on
>> the TV)
>>
>> Even if we did want to gate it on TV, I think the idea is to move away
>> from MV gating inter broker protocols. Ideally we can get to a state where
>> MV is just used for metadata changes.
>>
>> I think some of this discussion might fit more with the feature version
>> KIP, so I can try to open that up soon. Until we settle that, some of the
>> work in KIP-890 is blocked.
>>
>> Justine
>>
>> On Mon, Feb 5, 2024 at 5:38 PM Jun Rao  wrote:
>>
>>> Hi, Justine,
>>>
>>> Thanks for the reply.
>>>
>>> Since AddPartitions is an inter broker request, will its version be gated
>>> only by TV or other features like MV too? For example, if we need to
>>> change
>>> the protocol for AddPartitions for reasons other than txn verification in
>>> the future, will the new version be gated by a new MV? If so, does
>>> downgrading a TV imply potential downgrade of MV too?
>>>
>>> Jun
>>>
>>>
>>>
>>> On Mon, Feb 5, 2024 at 5:07 PM Justine Olshan
>>> 
>>> wrote:
>>>
>>> > One TV gates the flexible feature version (no rpcs involved, only the
>>> > transactional records that should only be gated by TV)
>>> > Another TV gates the ability to turn on kip-890 part 2. This would
>>> gate the
>>> > version of Produce and EndTxn (likely only used by transactions), and
>>> > specifies a flag in AddPartitionsToTxn though the version is already
>>> used
>>> > without TV.
>>> >
>>> > I think the only concern is the Produce request and we could consider
>>> work
>>> > arounds similar to the AddPartitionsToTxn call.
>>> >
>>> > Justine
>>> >
>>> > On Mon, Feb 5, 2024 at 4:56 PM Jun Rao 
>>> wrote:
>>> >
>>> > > Hi, Justine,
>>> > >
>>> > > Which PRC/record protocols will TV guard? Going forward, will those
>>> > > PRC/record protocols only be guarded by TV and not by other features
>>> like
>>> > > MV?
>>> > >
>>> > > Thanks,
>>> > >
>>> > > Jun
>>> > >
>>> > > On Mon, Feb 5, 2024 at 2:41 PM Justine Olshan
>>> > >> > > >
>>> > > wrote:
>>> > >
>>> > > > Hi Jun,
>>> > > >
>>> > > > Sorry I think I misunderstood your question or answered
>>> incorrectly.
>>> > The
>>> > > TV
>>> > > > version should ideally be fully independent from MV.
>>> > > > At least for the changes I proposed, TV should not affect MV and MV
>>> > > should
>>> > > > not affect TV/
>>> > > >
>>> > > > I think if we downgrade TV, only that feature should downgrade.
>>> > Likewise
>>> > > > the same with MV. The finalizedFeatures should just reflect the
>>> feature
>>> > > > downgrade we made.
>>> > > >
>>> > > > I also plan to write a new KIP for managing the disk format and
>>> upgrade
>>> > > > tool as we will need new flags to support these features. That
>>> should
>>> > > help
>>> > > > clarify some things.
>>> > > >
>>> > > > Justine
>>> > > >
>>> > > > On Mon, Feb 5, 2024 at 11:03 AM Jun Rao 
>>> > > wrote:
>>> > > >
>>> > > > > Hi, Justine,
>>> > > > >
>>> > > > > Thanks for the reply.
>>> > > > >
>>> > > > > So, if we downgrade TV, we could implicitly downgrade another
>>> feature
>>> > > > (say
>>> > > > > MV) that has dependency (e.g. RPC). What would we return for
>>> > > > > FinalizedFeatures for MV in ApiVersionsResponse in that case?

[jira] [Created] (KAFKA-16446) Log slow controller events

2024-03-28 Thread David Arthur (Jira)

David Arthur created KAFKA-16446:


 Summary: Log slow controller events
 Key: KAFKA-16446
 URL: https://issues.apache.org/jira/browse/KAFKA-16446
 Project: Kafka
  Issue Type: Improvement
Reporter: David Arthur


Occasionally, we will see very high p99 controller event processing times. 
Unless DEBUG logs are enabled, it is impossible to see which events are slow. 

Typically this happens during controller startup/failover, though it can also 
happen sporadically when the controller gets overloaded.



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

Re: [DISCUSS] KIP-477: Add PATCH method for connector config in Connect REST API

2024-03-28 Thread Chris Egerton

 No further comments from me.

On Thu, Mar 28, 2024, 19:36 Ivan Yurchenko  wrote:

> Hello Chris,
>
> Thanks for your feedback. I created the jira and also updated the
> description a bit mentioning that other similar race scenarios exist and
> the KIP is not trying to solve them.
>
> Best,
> Ivan
>
> On Wed, Mar 27, 2024, at 17:08, Chris Egerton wrote:
> > Hi Ivan,
> >
> > Thanks for the updates. LGTM!
> >
> > RE atomicity: I think it should be possible and not _too_ invasive to
> > detect and handle these kinds of races by tracking the offset in the
> config
> > topic for connector configs and aborting an operation if that offset
> > changes between when the request was initiated and when the write to the
> > config topic will take place, but since the same kind of issue is also
> > possible with other connector operations (concurrent configuration PUTs,
> > for example) due to how validation is split out into a separate thread, I
> > agree that it's not worth blocking the KIP on fixing this.
> >
> > One final nit: Can you update the Jira ticket link in the KIP?
> >
> > Cheers,
> >
> > Chris
> >
> > On Wed, Mar 27, 2024 at 2:56 PM Ivan Yurchenko  wrote:
> >
> > > Hi,
> > >
> > > I updated the KIP with the two following changes:
> > > 1. Using `null` values as tombstone value for removing existing fields
> > > from configuration.
> > > 2. Added a note about the lack of 100% atomicity, which seems very
> > > difficult to achieve practically.
> > >
> > > Ivan
> > >
> > >
> > > On Tue, Mar 26, 2024, at 14:45, Ivan Yurchenko wrote:
> > > > Speaking of the Chris' comment
> > > >
> > > > > One thought that comes to mind is that sometimes it may be useful
> to
> > > > > explicitly remove properties from a connector configuration. We
> might
> > > > > permit this by allowing users to specify null (the JSON literal,
> not a
> > > > > string containing the characters "null") as the value for
> to-be-removed
> > > > > properties.
> > > >
> > > > This actually makes sense. AFAIU, `null` cannot be in the connector
> > > config since https://github.com/apache/kafka/pull/11333, so using it
> as a
> > > tombstone value is a good idea. I can update the KIP.
> > > >
> > > > Ivan
> > > >
> > > >
> > > > On Tue, Mar 26, 2024, at 14:19, Ivan Yurchenko wrote:
> > > > > Hi all,
> > > > >
> > > > > This KIP is a bit old now :) but I think its context hasn't changed
> > > much since then and the KIP is still valid. I would like to finally
> bring
> > > it to some conclusion.
> > > > >
> > > > > Best,
> > > > > Ivan
> > > > >
> > > > > On 2021/07/12 14:49:47 Chris Egerton wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > Know it's been a while for this KIP but in my personal experience
> > > the value
> > > > > > of a PATCH method in the REST API has actually increased over
> time
> > > and I'd
> > > > > > love to have this option for quick, stop-the-bleeding remediation
> > > efforts.
> > > > > >
> > > > > > One thought that comes to mind is that sometimes it may be
> useful to
> > > > > > explicitly remove properties from a connector configuration. We
> might
> > > > > > permit this by allowing users to specify null (the JSON literal,
> not
> > > a
> > > > > > string containing the characters "null") as the value for
> > > to-be-removed
> > > > > > properties.
> > > > > >
> > > > > > I'd love to see this change if you're still interested in driving
> > > it, Ivan.
> > > > > > Hopefully we can give it the attention it deserves in the
> upcoming
> > > months!
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > > Chris
> > > > > >
> > > > > > On Fri, Jun 28, 2019 at 4:56 AM Ivan Yurchenko 
> > > > > > wrote:
> > > > > >
> > > > > > > Thank you for your feedback Ryanne!
> > > > > > > These are all surely valid concerns and PATCH isn't really
> > > necessary or
> > > > > > > suitable for normal production configuration management.
> However,
> > > there are
> > > > > > > cases where quick patching of the configuration is useful,
> such as
> > > hot
> > > > > > > fixes of production or in development.
> > > > > > >
> > > > > > > Overall, the change itself is really tiny and if the
> cost-benefit
> > > balance
> > > > > > > is positive, I'd still like to drive it further.
> > > > > > >
> > > > > > > Ivan
> > > > > > >
> > > > > > > On Wed, 26 Jun 2019 at 17:45, Ryanne Dolan 
> > > wrote:
> > > > > > >
> > > > > > > > Ivan, I looked at adding PATCH a while ago as well. I decided
> > > not to
> > > > > > > pursue
> > > > > > > > the idea for a few reasons:
> > > > > > > >
> > > > > > > > 1) PATCH is still racy. For example, if you want to add a
> topic
> > > to the
> > > > > > > > "topics" property, you still need to read, modify, and write
> the
> > > existing
> > > > > > > > value. To handle this, you'd need to support atomic
> sub-document
> > > > > > > > operations, which I don't see happening.
> > > > > > > >
> > > > > > > > 2) A common pattern is to store your configurations in git or
> > > something,
> > > > > > > > and then

Re: [DISCUSS] KIP-477: Add PATCH method for connector config in Connect REST API

2024-03-28 Thread Ivan Yurchenko

Hello Chris,

Thanks for your feedback. I created the jira and also updated the description a 
bit mentioning that other similar race scenarios exist and the KIP is not 
trying to solve them.

Best,
Ivan

On Wed, Mar 27, 2024, at 17:08, Chris Egerton wrote:
> Hi Ivan,
> 
> Thanks for the updates. LGTM!
> 
> RE atomicity: I think it should be possible and not _too_ invasive to
> detect and handle these kinds of races by tracking the offset in the config
> topic for connector configs and aborting an operation if that offset
> changes between when the request was initiated and when the write to the
> config topic will take place, but since the same kind of issue is also
> possible with other connector operations (concurrent configuration PUTs,
> for example) due to how validation is split out into a separate thread, I
> agree that it's not worth blocking the KIP on fixing this.
> 
> One final nit: Can you update the Jira ticket link in the KIP?
> 
> Cheers,
> 
> Chris
> 
> On Wed, Mar 27, 2024 at 2:56 PM Ivan Yurchenko  wrote:
> 
> > Hi,
> >
> > I updated the KIP with the two following changes:
> > 1. Using `null` values as tombstone value for removing existing fields
> > from configuration.
> > 2. Added a note about the lack of 100% atomicity, which seems very
> > difficult to achieve practically.
> >
> > Ivan
> >
> >
> > On Tue, Mar 26, 2024, at 14:45, Ivan Yurchenko wrote:
> > > Speaking of the Chris' comment
> > >
> > > > One thought that comes to mind is that sometimes it may be useful to
> > > > explicitly remove properties from a connector configuration. We might
> > > > permit this by allowing users to specify null (the JSON literal, not a
> > > > string containing the characters "null") as the value for to-be-removed
> > > > properties.
> > >
> > > This actually makes sense. AFAIU, `null` cannot be in the connector
> > config since https://github.com/apache/kafka/pull/11333, so using it as a
> > tombstone value is a good idea. I can update the KIP.
> > >
> > > Ivan
> > >
> > >
> > > On Tue, Mar 26, 2024, at 14:19, Ivan Yurchenko wrote:
> > > > Hi all,
> > > >
> > > > This KIP is a bit old now :) but I think its context hasn't changed
> > much since then and the KIP is still valid. I would like to finally bring
> > it to some conclusion.
> > > >
> > > > Best,
> > > > Ivan
> > > >
> > > > On 2021/07/12 14:49:47 Chris Egerton wrote:
> > > > > Hi all,
> > > > >
> > > > > Know it's been a while for this KIP but in my personal experience
> > the value
> > > > > of a PATCH method in the REST API has actually increased over time
> > and I'd
> > > > > love to have this option for quick, stop-the-bleeding remediation
> > efforts.
> > > > >
> > > > > One thought that comes to mind is that sometimes it may be useful to
> > > > > explicitly remove properties from a connector configuration. We might
> > > > > permit this by allowing users to specify null (the JSON literal, not
> > a
> > > > > string containing the characters "null") as the value for
> > to-be-removed
> > > > > properties.
> > > > >
> > > > > I'd love to see this change if you're still interested in driving
> > it, Ivan.
> > > > > Hopefully we can give it the attention it deserves in the upcoming
> > months!
> > > > >
> > > > > Cheers,
> > > > >
> > > > > Chris
> > > > >
> > > > > On Fri, Jun 28, 2019 at 4:56 AM Ivan Yurchenko 
> > > > > wrote:
> > > > >
> > > > > > Thank you for your feedback Ryanne!
> > > > > > These are all surely valid concerns and PATCH isn't really
> > necessary or
> > > > > > suitable for normal production configuration management. However,
> > there are
> > > > > > cases where quick patching of the configuration is useful, such as
> > hot
> > > > > > fixes of production or in development.
> > > > > >
> > > > > > Overall, the change itself is really tiny and if the cost-benefit
> > balance
> > > > > > is positive, I'd still like to drive it further.
> > > > > >
> > > > > > Ivan
> > > > > >
> > > > > > On Wed, 26 Jun 2019 at 17:45, Ryanne Dolan 
> > wrote:
> > > > > >
> > > > > > > Ivan, I looked at adding PATCH a while ago as well. I decided
> > not to
> > > > > > pursue
> > > > > > > the idea for a few reasons:
> > > > > > >
> > > > > > > 1) PATCH is still racy. For example, if you want to add a topic
> > to the
> > > > > > > "topics" property, you still need to read, modify, and write the
> > existing
> > > > > > > value. To handle this, you'd need to support atomic sub-document
> > > > > > > operations, which I don't see happening.
> > > > > > >
> > > > > > > 2) A common pattern is to store your configurations in git or
> > something,
> > > > > > > and then apply them via PUT. Throw in some triggers or jenkins
> > etc, and
> > > > > > you
> > > > > > > have a more robust solution than PATCH provides.
> > > > > > >
> > > > > > > 3) For properties that change a lot, it's possible to use an
> > out-of-band
> > > > > > > data source, e.g. Kafka or Zookeeper, and then have your
> > Connector
> > > > > > > subscribe to

[jira] [Created] (KAFKA-16445) PATCH method for connecto configuration

2024-03-28 Thread Ivan Yurchenko (Jira)

Ivan Yurchenko created KAFKA-16445:
--

 Summary: PATCH method for connecto configuration
 Key: KAFKA-16445
 URL: https://issues.apache.org/jira/browse/KAFKA-16445
 Project: Kafka
  Issue Type: Improvement
  Components: connect
Reporter: Ivan Yurchenko
Assignee: Ivan Yurchenko


As  [KIP-477: Add PATCH method for connector config in Connect REST 
API|https://cwiki.apache.org/confluence/display/KAFKA/KIP-477%3A+Add+PATCH+method+for+connector+config+in+Connect+REST+API]
 suggests, we should introduce the PATCH method for connector configuration.



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

[PR] MINOR: update 3.6 [kafka-site]

2024-03-28 Thread via GitHub



wcarlson5 opened a new pull request, #593:
URL: https://github.com/apache/kafka-site/pull/593

   Take the changes from https://github.com/apache/kafka/pull/14725/files and 
make them live for 3.6


-- 
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: dev-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [DISCUSS] KIP-853: KRaft Controller Membership Changes

Hi, Jose,

Thanks for the reply.

If I am adding a new voter and it takes a long time (because the new voter
is catching up), I'd want to know if the request is indeed being processed.
I thought that's the usage of uncommitted-voter-change.

Also, I am still not sure about having multiple brokers reporting the same
metric. For example, if they don't report the same value (e.g. because one
broker is catching up), how does a user know which value is correct?

Thanks,

Jun

On Thu, Mar 28, 2024 at 10:56 AM José Armando García Sancio
 wrote:

> Hi Jun,
>
> On Thu, Mar 28, 2024 at 10:35 AM Jun Rao  wrote:
> > The following are the steps of AddVoter. The bulk of the time is probably
> > in step 5, but the updated VotersRecord won't be written until step 6.
> So,
> > ideally, the controller leader should report the pending voter as soon as
> > step 1. The brokers and non-leader controllers can't do that until after
> > step 6. Having multiple brokers report the same metric can be confusing
> > when there is inconsistency.
>
> First, the replicas (leader, following voters and observers) will
> compute the metrics the same way. In other words, in the leader the
> uncommitted-voter-change metric will be true (1) from step 7 to after
> step 8.
>
> I added the metric to indicate to the operator if there are replicas
> that have updated their voters set to a value that is uncommitted
> value. The leader doesn't update its voters set until step 7.
>
> I don't think that we should add metrics to track the state of a
> specific RPC. Or if we do, it should be a seperate KIP where we have a
> mechanism for consistently tracking this state across all admin RPCs.
> What do you think?
>
> Thanks,
> --
> -José
>

[jira] [Created] (KAFKA-16444) Run KIP-848 unit tests under code coverage

Kirk True created KAFKA-16444:
-

 Summary: Run KIP-848 unit tests under code coverage
 Key: KAFKA-16444
 URL: https://issues.apache.org/jira/browse/KAFKA-16444
 Project: Kafka
  Issue Type: Task
  Components: clients, consumer, unit tests
Affects Versions: 3.7.0
Reporter: Kirk True
Assignee: Kirk True
 Fix For: 3.8.0






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

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

2024-03-28 Thread Justine Olshan

Hi Jun,

For both transaction state and group coordinator state, there are only
version 0 records.
KIP-915 introduced flexible versions, but it was never put to use. MV has
never gated these. This KIP will do that. I can include this context in the
KIP.

I'm happy to modify his 1 and 2 to 0 and 1.

Justine

On Thu, Mar 28, 2024 at 10:57 AM Jun Rao  wrote:

> Hi, David,
>
> Thanks for the reply.
>
> Historically, the format of all records were controlled by MV. Now, records
> in _offset_commit will be controlled by `group.coordinator.version`, is
> that right? It would be useful to document that.
>
> Also, we should align on the version numbering. "kafka-feature disable"
> says "Disable one or more feature flags. This is the same as downgrading
> the version to zero". So, in the `group.coordinator.version' case, we
> probably should use version 0 for the old consumer protocol.
>
> Jun
>
> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield <
> andrew_schofield_j...@outlook.com> wrote:
>
> > Hi David,
> > I agree that we should use the same mechanism to gate KIP-932 once that
> > feature reaches production readiness. The precise details of the values
> > will
> > depend upon the current state of all these flags when that release comes.
> >
> > Thanks,
> > Andrew
> >
> > > On 28 Mar 2024, at 07:11, David Jacot 
> > wrote:
> > >
> > > Hi, Jun, Justine,
> > >
> > > Regarding `group.coordinator.version`, the idea is to use it to gate
> > > records and APIs of the group coordinator. The first use case will be
> > > KIP-848. We will use version 2 of the flag to gate all the new records
> > and
> > > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So
> > version
> > > 1 will be the only the old protocol and version 2 will be the currently
> > > implemented new protocol. I don't think that we have any dependency on
> > the
> > > metadata version at the moment. The changes are orthogonal. I think
> that
> > we
> > > could mention KIP-848 as the first usage of this flag in the KIP. I
> will
> > > also update KIP-848 to include it when this KIP is accepted. Another
> use
> > > case is the Queues KIP. I think that we should also use this new flag
> to
> > > gate it.
> > >
> > > Best,
> > > David
> > >
> > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao 
> > wrote:
> > >
> > >> Hi, Justine,
> > >>
> > >> Thanks for the reply.
> > >>
> > >> So, "dependencies" and "version-mapping" will be added to both
> > >> kafka-feature and kafka-storage? Could we document that in the tool
> > format
> > >> section?
> > >>
> > >> Jun
> > >>
> > >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan
> > >> 
> > >> wrote:
> > >>
> > >>> Ok. I can remove the info from the describe output.
> > >>>
> > >>> Dependencies is needed for the storage tool because we want to make
> > sure
> > >>> the desired versions we are setting will be valid. Version mapping
> > should
> > >>> be for both tools since we have --release-version for both tools.
> > >>>
> > >>> I was considering changing the IV strings, but I wasn't sure if there
> > >> would
> > >>> be some disagreement with the decision. Not sure if that breaks
> > >>> compatibility etc. Happy to hear everyone's thoughts.
> > >>>
> > >>> Justine
> > >>>
> > >>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao 
> > >> wrote:
> > >>>
> >  Hi, Justine,
> > 
> >  Thanks for the reply.
> > 
> >  Having "kafka-feature dependencies" seems enough to me. We don't
> need
> > >> to
> >  include the dependencies in the output of "kafka-feature describe".
> > 
> >  We only support "dependencies" in kafka-feature, not kafka-storage.
> We
> >  probably should do the same for "version-mapping".
> > 
> >  bin/kafka-features.sh downgrade --feature metadata.version=16
> >  --transaction.protocol.version=2
> >  We need to add the --feature flag for the second feature, right?
> > 
> >  In "kafka-features.sh describe", we only show the IV string for
> >  metadata.version. Should we also show the level number?
> > 
> >  Thanks,
> > 
> >  Jun
> > 
> >  On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan
> >  
> >  wrote:
> > 
> > > I had already included this example
> > > bin/kafka-features.sh downgrade --feature metadata.version=16
> > > --transaction.protocol.version=2 // Throws error if metadata
> version
> > >>> is <
> > > 16, and this would be an upgrade
> > > But I have updated the KIP to explicitly say the text you
> mentioned.
> > >
> > > Justine
> > >
> > > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio
> > >  wrote:
> > >
> > >> Hi Justine,
> > >>
> > >> See my comment below.
> > >>
> > >> On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan
> > >>  wrote:
> > >>> The feature command includes the upgrade or downgrade command
> > >> along
> > > with
> > >>> the --release-version flag. If some features are not moving in
> > >> the
> >

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

Hi, David,

Thanks for the reply.

Historically, the format of all records were controlled by MV. Now, records
in _offset_commit will be controlled by `group.coordinator.version`, is
that right? It would be useful to document that.

Also, we should align on the version numbering. "kafka-feature disable"
says "Disable one or more feature flags. This is the same as downgrading
the version to zero". So, in the `group.coordinator.version' case, we
probably should use version 0 for the old consumer protocol.

Jun

On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield <
andrew_schofield_j...@outlook.com> wrote:

> Hi David,
> I agree that we should use the same mechanism to gate KIP-932 once that
> feature reaches production readiness. The precise details of the values
> will
> depend upon the current state of all these flags when that release comes.
>
> Thanks,
> Andrew
>
> > On 28 Mar 2024, at 07:11, David Jacot 
> wrote:
> >
> > Hi, Jun, Justine,
> >
> > Regarding `group.coordinator.version`, the idea is to use it to gate
> > records and APIs of the group coordinator. The first use case will be
> > KIP-848. We will use version 2 of the flag to gate all the new records
> and
> > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So
> version
> > 1 will be the only the old protocol and version 2 will be the currently
> > implemented new protocol. I don't think that we have any dependency on
> the
> > metadata version at the moment. The changes are orthogonal. I think that
> we
> > could mention KIP-848 as the first usage of this flag in the KIP. I will
> > also update KIP-848 to include it when this KIP is accepted. Another use
> > case is the Queues KIP. I think that we should also use this new flag to
> > gate it.
> >
> > Best,
> > David
> >
> > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao 
> wrote:
> >
> >> Hi, Justine,
> >>
> >> Thanks for the reply.
> >>
> >> So, "dependencies" and "version-mapping" will be added to both
> >> kafka-feature and kafka-storage? Could we document that in the tool
> format
> >> section?
> >>
> >> Jun
> >>
> >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan
> >> 
> >> wrote:
> >>
> >>> Ok. I can remove the info from the describe output.
> >>>
> >>> Dependencies is needed for the storage tool because we want to make
> sure
> >>> the desired versions we are setting will be valid. Version mapping
> should
> >>> be for both tools since we have --release-version for both tools.
> >>>
> >>> I was considering changing the IV strings, but I wasn't sure if there
> >> would
> >>> be some disagreement with the decision. Not sure if that breaks
> >>> compatibility etc. Happy to hear everyone's thoughts.
> >>>
> >>> Justine
> >>>
> >>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao 
> >> wrote:
> >>>
>  Hi, Justine,
> 
>  Thanks for the reply.
> 
>  Having "kafka-feature dependencies" seems enough to me. We don't need
> >> to
>  include the dependencies in the output of "kafka-feature describe".
> 
>  We only support "dependencies" in kafka-feature, not kafka-storage. We
>  probably should do the same for "version-mapping".
> 
>  bin/kafka-features.sh downgrade --feature metadata.version=16
>  --transaction.protocol.version=2
>  We need to add the --feature flag for the second feature, right?
> 
>  In "kafka-features.sh describe", we only show the IV string for
>  metadata.version. Should we also show the level number?
> 
>  Thanks,
> 
>  Jun
> 
>  On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan
>  
>  wrote:
> 
> > I had already included this example
> > bin/kafka-features.sh downgrade --feature metadata.version=16
> > --transaction.protocol.version=2 // Throws error if metadata version
> >>> is <
> > 16, and this would be an upgrade
> > But I have updated the KIP to explicitly say the text you mentioned.
> >
> > Justine
> >
> > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio
> >  wrote:
> >
> >> Hi Justine,
> >>
> >> See my comment below.
> >>
> >> On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan
> >>  wrote:
> >>> The feature command includes the upgrade or downgrade command
> >> along
> > with
> >>> the --release-version flag. If some features are not moving in
> >> the
> >>> direction mentioned (upgrade or downgrade) the command will fail
> >> --
> >> perhaps
> >>> with an error of which features were going in the wrong
> >> direction.
> >>
> >> How about updating the KIP to show and document this behavior?
> >>
> >> Thanks,
> >> --
> >> -José
> >>
> >
> 
> >>>
> >>
>
>

Re: [DISCUSS] KIP-853: KRaft Controller Membership Changes

2024-03-28 Thread José Armando García Sancio

Hi Jun,

On Thu, Mar 28, 2024 at 10:35 AM Jun Rao  wrote:
> The following are the steps of AddVoter. The bulk of the time is probably
> in step 5, but the updated VotersRecord won't be written until step 6. So,
> ideally, the controller leader should report the pending voter as soon as
> step 1. The brokers and non-leader controllers can't do that until after
> step 6. Having multiple brokers report the same metric can be confusing
> when there is inconsistency.

First, the replicas (leader, following voters and observers) will
compute the metrics the same way. In other words, in the leader the
uncommitted-voter-change metric will be true (1) from step 7 to after
step 8.

I added the metric to indicate to the operator if there are replicas
that have updated their voters set to a value that is uncommitted
value. The leader doesn't update its voters set until step 7.

I don't think that we should add metrics to track the state of a
specific RPC. Or if we do, it should be a seperate KIP where we have a
mechanism for consistently tracking this state across all admin RPCs.
What do you think?

Thanks,
-- 
-José

[jira] [Created] (KAFKA-16443) Update streams_static_membership_test.py to support KIP-848’s group protocol config

Kirk True created KAFKA-16443:
-

 Summary: Update streams_static_membership_test.py to support 
KIP-848’s group protocol config
 Key: KAFKA-16443
 URL: https://issues.apache.org/jira/browse/KAFKA-16443
 Project: Kafka
  Issue Type: Test
  Components: clients, consumer, system tests
Affects Versions: 3.7.0
Reporter: Kirk True
Assignee: Kirk True
 Fix For: 4.0.0


This task is to update the test method(s) in 
{{streams_standby_replica_test.py}} to support the {{group.protocol}} 
configuration introduced in 
[KIP-848|https://cwiki.apache.org/confluence/display/KAFKA/KIP-848%3A+The+Next+Generation+of+the+Consumer+Rebalance+Protocol]
 by adding an optional {{group_protocol}} argument to the tests and matrixes.

See KAFKA-16231 as an example of how the test parameters can be changed.



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

[jira] [Created] (KAFKA-16442) Update streams_standby_replica_test.py to support KIP-848’s group protocol config

Kirk True created KAFKA-16442:
-

 Summary: Update streams_standby_replica_test.py to support 
KIP-848’s group protocol config
 Key: KAFKA-16442
 URL: https://issues.apache.org/jira/browse/KAFKA-16442
 Project: Kafka
  Issue Type: Test
  Components: clients, consumer, system tests
Affects Versions: 3.7.0
Reporter: Kirk True
Assignee: Kirk True
 Fix For: 4.0.0


This task is to update the test method(s) in {{security_test.py}} to support 
the {{group.protocol}} configuration introduced in 
[KIP-848|https://cwiki.apache.org/confluence/display/KAFKA/KIP-848%3A+The+Next+Generation+of+the+Consumer+Rebalance+Protocol]
 by adding an optional {{group_protocol}} argument to the tests and matrixes.

See KAFKA-16231 as an example of how the test parameters can be changed.



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

[jira] [Created] (KAFKA-16440) Update security_test.py to support KIP-848’s group protocol config

Kirk True created KAFKA-16440:
-

 Summary: Update security_test.py to support KIP-848’s group 
protocol config
 Key: KAFKA-16440
 URL: https://issues.apache.org/jira/browse/KAFKA-16440
 Project: Kafka
  Issue Type: Test
  Components: clients, consumer, system tests
Affects Versions: 3.7.0
Reporter: Kirk True
Assignee: Kirk True
 Fix For: 3.8.0


This task is to update the test method(s) in 
{{replication_replica_failure_test.py}} to support the {{group.protocol}} 
configuration introduced in 
[KIP-848|https://cwiki.apache.org/confluence/display/KAFKA/KIP-848%3A+The+Next+Generation+of+the+Consumer+Rebalance+Protocol]
 by adding an optional {{group_protocol}} argument to the tests and matrixes.

See KAFKA-16231 as an example of how the test parameters can be changed.



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

[jira] [Created] (KAFKA-16441) Update streams_broker_down_resilience_test.py to support KIP-848’s group protocol config

Kirk True created KAFKA-16441:
-

 Summary: Update streams_broker_down_resilience_test.py to support 
KIP-848’s group protocol config
 Key: KAFKA-16441
 URL: https://issues.apache.org/jira/browse/KAFKA-16441
 Project: Kafka
  Issue Type: Test
  Components: clients, consumer, system tests
Affects Versions: 3.7.0
Reporter: Kirk True
Assignee: Kirk True
 Fix For: 3.8.0


This task is to update the test method(s) in {{security_test.py}} to support 
the {{group.protocol}} configuration introduced in 
[KIP-848|https://cwiki.apache.org/confluence/display/KAFKA/KIP-848%3A+The+Next+Generation+of+the+Consumer+Rebalance+Protocol]
 by adding an optional {{group_protocol}} argument to the tests and matrixes.

See KAFKA-16231 as an example of how the test parameters can be changed.



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

[jira] [Created] (KAFKA-16439) Update replication_replica_failure_test.py to support KIP-848’s group protocol config

Kirk True created KAFKA-16439:
-

 Summary: Update replication_replica_failure_test.py to support 
KIP-848’s group protocol config
 Key: KAFKA-16439
 URL: https://issues.apache.org/jira/browse/KAFKA-16439
 Project: Kafka
  Issue Type: Test
  Components: clients, consumer, system tests
Affects Versions: 3.7.0
Reporter: Kirk True
Assignee: Kirk True
 Fix For: 3.8.0


This task is to update the test method(s) in {{replica_scale_test.py}} to 
support the {{group.protocol}} configuration introduced in 
[KIP-848|https://cwiki.apache.org/confluence/display/KAFKA/KIP-848%3A+The+Next+Generation+of+the+Consumer+Rebalance+Protocol]
 by adding an optional {{group_protocol}} argument to the tests and matrixes.

See KAFKA-16231 as an example of how the test parameters can be changed.



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

[jira] [Created] (KAFKA-16438) Update consumer_test.py’s static tests to support KIP-848’s group protocol config

Kirk True created KAFKA-16438:
-

 Summary: Update consumer_test.py’s static tests to support 
KIP-848’s group protocol config
 Key: KAFKA-16438
 URL: https://issues.apache.org/jira/browse/KAFKA-16438
 Project: Kafka
  Issue Type: Bug
  Components: clients, consumer, system tests
Reporter: Kirk True
Assignee: Kirk True


This task is to update the following test method(s) in {{consumer_test.py}} to 
support the {{group.protocol}} configuration:

* {{test_fencing_static_consumer}}
* {{test_static_consumer_bounce}}
* {{test_static_consumer_persisted_after_rejoin}}



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

[jira] [Resolved] (KAFKA-16271) Update consumer_rolling_upgrade_test.py to support KIP-848’s group protocol config



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

Kirk True resolved KAFKA-16271.
---
  Reviewer: Lucas Brutschy
Resolution: Fixed

> Update consumer_rolling_upgrade_test.py to support KIP-848’s group protocol 
> config
> --
>
> Key: KAFKA-16271
> URL: https://issues.apache.org/jira/browse/KAFKA-16271
> Project: Kafka
>  Issue Type: Test
>  Components: clients, consumer, system tests
>Affects Versions: 3.7.0
>Reporter: Kirk True
>Assignee: Philip Nee
>Priority: Blocker
>  Labels: kip-848-client-support, system-tests
> Fix For: 3.8.0
>
>
> This task is to update the test method(s) in 
> {{consumer_rolling_upgrade_test.py}} to support the {{group.protocol}} 
> configuration introduced in 
> [KIP-848|https://cwiki.apache.org/confluence/display/KAFKA/KIP-848%3A+The+Next+Generation+of+the+Consumer+Rebalance+Protocol]
>  by adding an optional {{group_protocol}} argument to the tests and matrixes.
> See KAFKA-16231 as an example of how the test parameters can be changed.
> The tricky wrinkle here is that the existing test relies on client-side 
> assignment strategies that aren't applicable with the new KIP-848-enabled 
> consumer.



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

Re: [DISCUSS] KIP-853: KRaft Controller Membership Changes

Hi, Jose,

Thanks for the reply.

The following are the steps of AddVoter. The bulk of the time is probably
in step 5, but the updated VotersRecord won't be written until step 6. So,
ideally, the controller leader should report the pending voter as soon as
step 1. The brokers and non-leader controllers can't do that until after
step 6. Having multiple brokers report the same metric can be confusing
when there is inconsistency.

1. Wait until there are no uncommitted VotersRecord. Note that the
implementation may just return a REQUEST_TIMED_OUT error if there are
pending operations.
2. Wait for the LeaderChangeMessage control record from the current epoch
to get committed. Note that the implementation may just return a
REQUEST_TIMED_OUT error if there are pending operations.
3. Send an ApiVersions RPC to the first listener to discover the supported
kraft.version of the new voter.
4. Check that the new voter supports the current kraft.version.
5. Wait for the fetch offset of the replica (ID, UUID) to catch up to the
log end offset of the leader.
6. Append the updated VotersRecord to the log.
7. The KRaft internal listener will read this record from the log and add
the voter to the voters set.
8. Wait for the VotersRecord to commit using the majority of new voters
set. Return a REQUEST_TIMED_OUT error if it doesn't succeed in time.
9. Send the AddVoter response to the client.

Jun

On Wed, Mar 27, 2024 at 7:52 PM José Armando García Sancio
 wrote:

> Hi Jun,
>
> On Wed, Mar 27, 2024 at 2:26 PM Jun Rao  wrote:
> > 55.1 How does the broker and non-leader controller know the pending
> voters?
>
> They are in the log. Pending voter sets are VotersRecords between the
> HWM and the LEO. The leader will make sure that there is at most one
> VoterRecord that is uncommitted (between the HWM and LEO).
>
> Maybe uncommitted-voter-change is a better name. Updated the KIP to
> use this name.
>
> Thanks,
> --
> -José
>

Re: [DISCUSS] KIP-1024: Make the restore behavior of GlobalKTables with custom processors configureable

2024-03-28 Thread Walker Carlson

Hey all,

Thanks for the feedback Bruno, Almog and Matthias!

Almog: I like the idea, but I agree with Matthais. I actually looked at
that ticket a bit when doing this and found that while similar they are
actually pretty unrelated codewise. I would love to see it get taken care
of.

Bruno and Matthias: The Named parameter doesn't really make sense to me to
put it here. The store in the Store builder is already named through what
Matthais described and the processor doesn't actually have a name. That
would be the processor node that gets named via the Named parameter  (in
the DSL) and the internal streams builder uses the consumed object to make
a source name. I think we should just keep the Consumed object and used
that for the processor node name.

As for the limitation of the store builder interface I don't think it is
necessary. It could be something we add later if we really want to.

Anyways I think we are getting close enough to consensus that I'm going to
open a vote and hopefully we can get it voted on soon!

best,
Walker

On Thu, Mar 28, 2024 at 5:55 AM Matthias J. Sax  wrote:

> Hey,
>
> looking into the API, I am wondering why we would need to add an
> overload talking a `Named` parameter?
>
> StreamsBuilder.addGlobalStore() (and .addGlobalTable()) already takes a
> `Consumed` parameter that allows to set a name.
>
>
> > 2.
> > I do not understand what you mean with "maximum flexibility". The
> built-in processor needs to assume a given state store interface. That
> means, users have to provide a state store that offers that interface. If
> they do not they will get a runtime exception. If we require a store
> builder for a given interface, we can catch the mistake at compile time.
> Let me know whether I misunderstood something.
>
> Yes, we could catch it at runtime. But I guess what I was trying to say
> is different: I was trying to say, we should not limit the API to always
> require a specific store, such that global stores can only be of a
> certain type. Global Stores should be allowed to be of any type. Hence,
> if we add a built-in processor, it can only be one option, and we always
> need to support custom processor, and might also want to try to allow
> the restore optimization for custom processor (and thus other store
> types), not just for our built-in processor (and our built-in stores).
> Coupling the optimization to built-in stores would prevent us to apply
> the optimization to custom stores.
>
>
>
> @Almog: interesting idea. I tend to think that both issues are
> orthogonal. If users pick to apply the optimization "added" by this KIP,
> the bug you mentioned would still apply to global stores, and thus this
> KIP is not addressing the issue you mentioned.
>
> Personally, I also think that we don't need a KIP to fix the ticket you
> mentioned? In the end, we need to skip records during restore, and it
> seems it does not make sense to make this configurable?
>
>
>
> -Matthias
>
>
> On 3/26/24 4:24 PM, Almog Gavra wrote:
> > Thanks for the thoughts Bruno!
> >
> >> Do you mean a API to configure restoration instead of boolean flag
> > reprocessOnRestore?
> >
> > Yes, this is exactly the type of thing I was musing (but I don't have any
> > concrete suggestions). It feels like that would give the flexibility to
> do
> > things like the motivation section of the KIP (allow bulk loading of
> > records without reprocessing) while also solving other limitations.
> >
> > I'm supportive of the KIP as-is but was hoping somebody with more
> > experience would have a sudden inspiration for how to solve both issues
> > with one API! Anyway, I'll slide back into the lurking shadows for now
> and
> > let the discussion continue :)
> >
> > Cheers,
> > Almog
> >
> > On Tue, Mar 26, 2024 at 4:22 AM Bruno Cadonna 
> wrote:
> >
> >> Hi Almog,
> >>
> >> Do you mean a API to configure restoration instead of boolean flag
> >> reprocessOnRestore?
> >>
> >> Do you already have an idea?
> >>
> >> The proposal in the KIP is focused on the processor that updates the
> >> global state whereas in the case of GlobalKTable and source KTable the
> >> issues lies in the deserialization of records from the input topics, but
> >> only if the deserialization error handler is configured to drop the
> >> problematic record. Additionally, for source KTable the source topic
> >> optimization must be turned on to run into the issue. I am wondering how
> >> a unified API for global stores, GlobalKTable, and source KTable might
> >> look like.
> >>
> >> While it is an interesting question, I am in favor of deferring this to
> >> a separate KIP.
> >>
> >> Best,
> >> Bruno
> >>
> >> On 3/26/24 12:49 AM, Almog Gavra wrote:
> >>> Hello Folk!
> >>>
> >>> Glad to see improvements to the GlobalKTables in discussion! I think
> they
> >>> deserve more love :)
> >>>
> >>> Scope creep alert (which I'm generally against and certainly still
> >> support
> >>> this KIP without but I want to see if there's an elegant way to address
>

Re: [DISCUSS] KIP-816: Topology changes without local state reset

Hi everyone,

I'm going to resurrect this KIP, because I would like the community to
benefit from our solution.

In the end, we internally solved this problem using Option B: automatically
moving state directories to the correct location whenever they're no longer
aligned with the Topology. We implemented this for ourselves externally to
Kafka Streams, by using Topology#describe() to analyse the Topology, and
then moving state directories before calling KafkaStreams#start().

I've updated/re-written the KIP to focus on this solution, albeit properly
integrated into Kafka Streams.

Let me know what you think,

Nick

On Tue, 15 Feb 2022 at 16:23, Nick Telford  wrote:

> In the KIP, for Option A I suggested a new path of:
>
> /state/dir/stores//
>
> I made the mistake of thinking that the rocksdb/ segment goes *after* the
> store name in the current scheme, e.g.
>
> /state/dir//[/rocksdb]
>
> This is a mistake. I'd always intended for a combination of the store name
> and partition number to be encoded in the new path (instead of the store
> name and task ID, that we have now). The exact encoding doesn't really
> bother me too much, so if you have any conventions you think we should
> follow here (hyphenated vs. underscored vs. directory separator, etc.)
> please let me know.
>
> I should be able to find some time hopefully next week to start working on
> this, which should shed some more light on issues that might arise.
>
> In the meantime I'll correct the KIP to include the rocksdb segment.
>
> Thanks everyone for your input so far!
>
> Nick
>
> On Mon, 14 Feb 2022 at 22:02, Guozhang Wang  wrote:
>
>> Thanks for the clarification John!
>>
>> Nick, sorry that I was not super clear in my latest email. I meant exactly
>> what John said.
>>
>> Just to clarify, I do think that this KIP is relatively orthogonal to the
>> named topology work; as long as we still keep the topo name encoded it
>> should be fine since two named topologies can indeed have the same store
>> name, but that would not need to be considered as part of this KIP.
>>
>>
>> Guozhang
>>
>> On Mon, Feb 14, 2022 at 9:02 AM John Roesler  wrote:
>>
>> > Hi Nick,
>> >
>> > When Guozgang and I were chatting, we realized that it’s not completely
>> > sufficient just to move the state store directories, because their names
>> > are not unique. In particular, more than one partition of the store may
>> be
>> > assigned to the same instance. Right now, this is handled because the
>> task
>> > is encoded the partition number.
>> >
>> > For example, if we have a store "mystore" in subtopology 1 and we have
>> two
>> > out of four partitions (0 and 3) assigned to the local node, the disk
>> will
>> > have these paths:
>> >
>> > {app_id}/1_0/rocksdb/mystore
>> > {app_id}/1_3/rocksdb/mystore
>> >
>> > Clearly, we can't just elevate both "mystore" directories to reside
>> under
>> > {appid}, because
>> > they have the same name. When I think of option (A), here's what I
>> picture:
>> >
>> > {app_id}/rocksdb/mystore-0
>> > {app_id}/rocksdb/mystore-3
>> >
>> > In the future, one thing we're considering to do is actually store all
>> the
>> > positions in the same rocksDB database, which is a pretty convenient
>> step
>> > away from option (A) (another reason to prefer it to option (B) ).
>> >
>> > I just took a look at how named topologies are handled, and they're
>> > actually
>> > a separate path segment, not part of the task id, like this:
>> >
>> > {app_id}/__{topo_name}__/1_0/rocksdb/mystore
>> > {app_id}/__{topo_name}__/1_3/rocksdb/mystore
>> >
>> > Which is pretty convenient because it means there are no
>> > implications for your proposal. If you implement the above
>> > code, then we'll just wind up with:
>> >
>> > {app_id}/__{topo_name}__/rocksdb/mystore-0
>> > {app_id}/__{topo_name}__/rocksdb/mystore-3
>> >
>> > Does that make sense?
>> >
>> > Thanks,
>> > -John
>> >
>> >
>> > On Mon, Feb 14, 2022, at 03:57, Nick Telford wrote:
>> > > Hi Guozhang,
>> > >
>> > > Sorry I haven't had the time to respond to your earlier email, but I
>> just
>> > > wanted to clarify something with respect to your most recent email.
>> > >
>> > > My original plan in option A is to remove the entire Task ID from the
>> > State
>> > > Store path, which would insulate it from any changes to the Task ID
>> > format
>> > > introduced by Named Topologies or anything else. This would in fact
>> > > consolidate the store for the instance, rather than by-Task (which I
>> > think
>> > > is what you meant by "one physical store per state"?).
>> > >
>> > > I did highlight in option C the possibility of changing the format of
>> the
>> > > Task ID to change the sub-topology ID from an ordinal to a stable
>> > > identifier. Although I'm not convinced that this option is viable, or
>> > even
>> > > desirable.
>> > >
>> > > Regards,
>> > >
>> > > Nick
>> > >
>> > > On Sat, 12 Feb 2022 at 00:36, Guozhang Wang 
>> wrote:
>> > >
>> > > > Just to follow-up on this thread, I had another chat

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

Quick addendum:

My suggested metric "oldest-open-iterator-age-seconds" should be
"oldest-open-iterator-age-ms". Milliseconds is obviously a better
granularity for such a metric.

Still accepting suggestions for a better name.

On Thu, 28 Mar 2024 at 13:41, Nick Telford  wrote:

> Hi everyone,
>
> Sorry for leaving this for so long. So much for "3 weeks until KIP freeze"!
>
> On Sophie's comments:
> 1. Would Matthias's suggestion of a separate metric tracking the age of
> the oldest open iterator (within the tag set) satisfy this? That way we can
> keep iterator-duration-(avg|max) for closed iterators, which can be useful
> for performance debugging for iterators that don't leak. I'm not sure what
> we'd call this metric, maybe: "oldest-open-iterator-age-seconds"? Seems
> like a mouthful.
>
> 2. You're right, it makes more sense to provide
> iterator-duration-(avg|max). Honestly, I can't remember why I had "total"
> before, or why I was computing a rate-of-change over it.
>
> 3, 4, 5, 6. Agreed, I'll make all those changes as suggested.
>
> 7. Combined with Matthias's point about RocksDB, I'm convinced that this
> is the wrong KIP for these. I'll introduce the additional Rocks metrics in
> another KIP.
>
> On Matthias's comments:
> A. Not sure about the time window. I'm pretty sure all existing avg/max
> metrics are since the application was started? Any other suggestions here
> would be appreciated.
>
> B. Agreed. See point 1 above.
>
> C. Good point. My focus was very much on Rocks memory leaks when I wrote
> the first draft. I can generalise it. My only concern is that it might make
> it more difficult to detect Rocks iterator leaks caused *within* our
> high-level iterator, e.g. RocksJNI, RocksDB, RocksDBStore, etc. But we
> could always provide a RocksDB-specific metric for this, as you suggested.
>
> D. Hmm, we do already have MeteredKeyValueIterator, which automatically
> wraps the iterator from inner-stores of MeteredKeyValueStore. If we
> implemented these metrics there, then custom stores would automatically
> gain the functionality, right? This seems like a pretty logical place to
> implement these metrics, since MeteredKeyValueStore is all about adding
> metrics to state stores.
>
> > I imagine the best way to implement this would be to do so at the
> > high-level iterator rather than implementing it separately for each
> > specific iterator implementation for every store type.
>
> Sophie, does MeteredKeyValueIterator fit with your recommendation?
>
> Thanks for your thoughts everyone, I'll update the KIP now.
>
> Nick
>
> On Thu, 14 Mar 2024 at 03:37, Sophie Blee-Goldman 
> wrote:
>
>> About your last two points: I completely agree that we should try to
>> make this independent of RocksDB, and should probably adopt a
>> general philosophy of being store-implementation agnostic unless
>> there is good reason to focus on a particular store type: eg if it was
>> only possible to implement for certain stores, or only made sense in
>> the context of a certain store type but not necessarily stores in general.
>>
>> While leaking memory due to unclosed iterators on RocksDB stores is
>> certainly the most common issue, I think Matthias sufficiently
>> demonstrated that the problem of leaking iterators is not actually
>> unique to RocksDB, and we should consider including in-memory
>> stores at the very least. I also think that at this point, we may as well
>> just implement the metrics for *all* store types, whether rocksdb or
>> in-memory or custom. Not just because it probably applies to all
>> store types (leaking iterators are rarely a good thing!) but because
>> I imagine the best way to implement this would be to do so at the
>> high-level iterator rather than implementing it separately for each
>> specific iterator implementation for every store type.
>>
>> That said, I haven't thought all that carefully about the implementation
>> yet -- it just strikes me as easiest to do at the top level of the store
>> hierarchy rather than at the bottom. My gut instinct may very well be
>> wrong, but that's what it's saying
>>
>> On Thu, Mar 7, 2024 at 10:43 AM Matthias J. Sax  wrote:
>>
>> > Seems I am late to this party. Can we pick this up again aiming for 3.8
>> > release? I think it would be a great addition. Few comments:
>> >
>> >
>> > - I think it does make sense to report `iterator-duration-avg` and
>> > `iterator-duration-max` for all *closed* iterators -- it just seems to
>> > be a useful metric (wondering if this would be _overall_ or bounded to
>> > some time window?)
>> >
>> > - About the duration iterators are currently open, I believe the only
>> > useful way is to report the "oldest iterator", ie, the smallest iterator
>> > open-time, of all currently open-iterator? We all agree that in general,
>> > leaking iterator would bump the count metric, and if there is a few ones
>> > which are not closed and open for a long time, it seem sufficient to
>> > detect the single oldest one for

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

Hi everyone,

Sorry for leaving this for so long. So much for "3 weeks until KIP freeze"!

On Sophie's comments:
1. Would Matthias's suggestion of a separate metric tracking the age of the
oldest open iterator (within the tag set) satisfy this? That way we can
keep iterator-duration-(avg|max) for closed iterators, which can be useful
for performance debugging for iterators that don't leak. I'm not sure what
we'd call this metric, maybe: "oldest-open-iterator-age-seconds"? Seems
like a mouthful.

2. You're right, it makes more sense to provide
iterator-duration-(avg|max). Honestly, I can't remember why I had "total"
before, or why I was computing a rate-of-change over it.

3, 4, 5, 6. Agreed, I'll make all those changes as suggested.

7. Combined with Matthias's point about RocksDB, I'm convinced that this is
the wrong KIP for these. I'll introduce the additional Rocks metrics in
another KIP.

On Matthias's comments:
A. Not sure about the time window. I'm pretty sure all existing avg/max
metrics are since the application was started? Any other suggestions here
would be appreciated.

B. Agreed. See point 1 above.

C. Good point. My focus was very much on Rocks memory leaks when I wrote
the first draft. I can generalise it. My only concern is that it might make
it more difficult to detect Rocks iterator leaks caused *within* our
high-level iterator, e.g. RocksJNI, RocksDB, RocksDBStore, etc. But we
could always provide a RocksDB-specific metric for this, as you suggested.

D. Hmm, we do already have MeteredKeyValueIterator, which automatically
wraps the iterator from inner-stores of MeteredKeyValueStore. If we
implemented these metrics there, then custom stores would automatically
gain the functionality, right? This seems like a pretty logical place to
implement these metrics, since MeteredKeyValueStore is all about adding
metrics to state stores.

> I imagine the best way to implement this would be to do so at the
> high-level iterator rather than implementing it separately for each
> specific iterator implementation for every store type.

Sophie, does MeteredKeyValueIterator fit with your recommendation?

Thanks for your thoughts everyone, I'll update the KIP now.

Nick

On Thu, 14 Mar 2024 at 03:37, Sophie Blee-Goldman 
wrote:

> About your last two points: I completely agree that we should try to
> make this independent of RocksDB, and should probably adopt a
> general philosophy of being store-implementation agnostic unless
> there is good reason to focus on a particular store type: eg if it was
> only possible to implement for certain stores, or only made sense in
> the context of a certain store type but not necessarily stores in general.
>
> While leaking memory due to unclosed iterators on RocksDB stores is
> certainly the most common issue, I think Matthias sufficiently
> demonstrated that the problem of leaking iterators is not actually
> unique to RocksDB, and we should consider including in-memory
> stores at the very least. I also think that at this point, we may as well
> just implement the metrics for *all* store types, whether rocksdb or
> in-memory or custom. Not just because it probably applies to all
> store types (leaking iterators are rarely a good thing!) but because
> I imagine the best way to implement this would be to do so at the
> high-level iterator rather than implementing it separately for each
> specific iterator implementation for every store type.
>
> That said, I haven't thought all that carefully about the implementation
> yet -- it just strikes me as easiest to do at the top level of the store
> hierarchy rather than at the bottom. My gut instinct may very well be
> wrong, but that's what it's saying
>
> On Thu, Mar 7, 2024 at 10:43 AM Matthias J. Sax  wrote:
>
> > Seems I am late to this party. Can we pick this up again aiming for 3.8
> > release? I think it would be a great addition. Few comments:
> >
> >
> > - I think it does make sense to report `iterator-duration-avg` and
> > `iterator-duration-max` for all *closed* iterators -- it just seems to
> > be a useful metric (wondering if this would be _overall_ or bounded to
> > some time window?)
> >
> > - About the duration iterators are currently open, I believe the only
> > useful way is to report the "oldest iterator", ie, the smallest iterator
> > open-time, of all currently open-iterator? We all agree that in general,
> > leaking iterator would bump the count metric, and if there is a few ones
> > which are not closed and open for a long time, it seem sufficient to
> > detect the single oldest one for alerting purpose?
> >
> > - What I don't like about the KIP is it focus on RocksDB. I don't think
> > we should build on the internal RocksDB counters as proposed (I guess,
> > we could still expose them, similar to other RocksDB metrics which we
> > expose already). However, for this new metric, we should track it
> > ourselves and thus make it independent of RocksDB -- in the end, an
> > in-memory store could also

Re: [DISCUSS] KIP-990: Capability to SUSPEND Tasks on DeserializationException

Hi folks,

Sorry I haven't got back to you until now.

It's become clear that I hadn't anticipated a significant number of
technical challenges that this KIP presents. I think expecting users to
understand the ramifications on aggregations, joins and windowing
ultimately kills it: it only becomes a problem under *specific*
combinations of operations, and those problems would manifest in ways that
might be difficult for users to detect, let alone diagnose.

I think it's best to abandon this KIP, at least for now. If anyone else
sees a use for and path forwards for it, feel free to pick it up.

Since I'm abandoning the KIP, I won't update the Motivation section. But I
will provide a bit of background here on why I originally suggested it, in
case you're interested:

In my organisation, we don't have schemas for *any* of our data in Kafka.
Consequently, one of the biggest causes of downtime in our applications are
"bad" records being written by Producers. We integrate with a lot of
third-party APIs, and have Producers that just push that data straight to
Kafka with very little validation. I've lost count of the number of times
my application has been crashed by a deserialization exception because we
received a record that looks like '{"error": "Bad gatetway"}' or similar,
instead of the actual payload we expect.

The difficulty is we can't just use CONTINUE to discard these messages,
because we also sometimes get deserialization exceptions caused by an
upstream schema change that is incompatible with the expectations of our
app. In these cases, we don't want to discard records (which are
technically valid), but instead need to adjust our application to be
compatible with the new schema, before processing them.

Crucially, we use a monolithic app, with more than 45 sub-topologies, so
crashing the entire app just because of one bad record causes downtime on
potentially unrelated sub-topologies.

This was the motivation for this KIP, which would have enabled users to
make a decision on what to do about a bad message, *without taking down the
entire application*.

Obviously, the *correct* solution to this problem is to introduce schemas
on our topics and have our Producers correctly validate records before
writing them to the cluster. This is ultimately the solution I am going to
pursue in lieu of this KIP.

I still think this KIP could have been useful for dealing with an
incompatible upstream schema change; by pausing only the sub-topologies
that are affected by the schema change, while leaving others to continue to
run while the user deploys a fix. However, in practice I think few users
have monolithic apps like ours, and most instead de-couple unrelated topics
via different apps, which reduces the impact of incompatible upstream
schema changes.

Thanks for your reviews and feedback, I've learned a lot, as always; this
time, mostly about how, when authoring a KIP,  I should always ask myself:
"yes, but what about timestamp ordering?" :-D

Nick

On Thu, 14 Mar 2024 at 03:27, Sophie Blee-Goldman 
wrote:

> >
> > Well, the KIP mentions the ability to either re-try the record (eg,
> > after applying some external fix that would allow Kafka Streams to now
> > deserialize the record now) or to skip it by advancing the offset.
>
>
> That's fair -- you're definitely right that what's described in the KIP
> document
> right now would not be practical. I just wanted to clarify that this
> doesn't
> mean the feature as a whole is impractical, but certainly we'd want to
> update the proposal to remove the line about resetting offsets via external
> tool and come up with a more concrete approach, and perhaps  describe
> it in more detail.
>
> That's  probably not worth getting into until/unless we decide whether to
> go forward with this feature in the first place. I'll let Nick reflect on
> the
> motivation and your other comments and then decide whether he still
> wants to pursue it.
>
> To Nick: if you want to go through with this KIP and can expand on the
> motivation so that we understand it better, I'd be happy to help work
> out the details. For now I'll just wait for your decision
>
> On Wed, Mar 13, 2024 at 10:24 AM Matthias J. Sax  wrote:
>
> > Yes, about the "drop records" case. It's a very common scenario to have
> > a repartition step before a windowed aggregation or a join with
> > grace-period.
> >
> >
> > About "add feature vs guard users": it's always a tricky question and
> > tradeoff. For this particular KIP, I personally think we should opt to
> > not add the feature but guard the users, as I don't see too much value
> > compared to the complexity and "traps" it adds. -- It's of course just
> > my personal opinion, and if there is a asked from many users to add this
> > feature, I would not push back further. As mentioned in my previous
> > reply, I don't fully understand the motivation yet; maybe Nick can
> > provide more context on it.
> >
> >
> > > In other words, opting for the PAUSE option would

Assistance Needed with Creating Wiki ID for Kafka Contribution

2024-03-28 Thread Prashant Jagtap

Hi,

I hope this email finds you well.

I've been eager to contribute to Apache Kafka and have started following the 
procedure outlined in the Kafka Improvement Proposals (KIP) documentation - 
https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals#KafkaImprovementProposals-GettingStarted.
 I've successfully signed up for the Developer mailing list and created a Jira 
ID.
However, I've encountered a hurdle while trying to create a Wiki ID by 
following this url - https://cwiki.apache.org/confluence/signup.action. Despite 
the provided instructions, I'm unable to complete the sign-up process for the 
Wiki ID. Attached the screenshot for your reference.
Could you please assist me with the steps required to create a Wiki ID?
(I have already sent an email to infrastruct...@apache.org regarding the same)

Thank you for your time and support.

Thanks and Regards,
Prashant Jagtap

Re: [DISCUSS] KIP-1027 Add MockFixedKeyProcessorContext

2024-03-28 Thread Matthias J. Sax

It seems that `MockRecordMetadata` is a private class, and thus not part 
of the public API. If there are any changes required, we don't need to 
discuss on the KIP.



For `CapturedPunctuator` and `CapturedForward` it's a little bit more 
tricky. My gut feeling is, that the classes might not need to be 
changed, but if we use them within `MockProcessorContext` and 
`MockFixedKeyProcessorContext` it might be weird to keep the current 
nesting... The problem I see is, that it's not straightforward how to 
move the classes w/o breaking compatibility, nor if we duplicate them as 
standalone classes w/o a larger "splash radius". (We would need to add 
new overloads for MockProcessorContext#scheduledPunctuators() and 
MockProcessorContext#forwarded()).


Might be good to hear from others if we think it's worth this larger 
changes to get rid of the nesting, or just accept the somewhat not ideal 
nesting as it technically is not a real issue?



-Matthias


On 3/15/24 1:47 AM, Shashwat Pandey wrote:

Thanks for the feedback Matthias!

The reason I proposed the extension of MockProcessorContext was more to do
with the internals of the class (MockRecordMetadata, CapturedPunctuator and
CapturedForward).

However, I do see your point, I would then think to split
MockProcessorContext and MockFixedKeyProcessorContext, some of the internal
classes should also be extracted i.e. MockRecordMetadata,
CapturedPunctuator and probably a new CapturedFixedKeyForward.

Let me know what you think!


Regards,
Shashwat Pandey


On Mon, Mar 11, 2024 at 10:09 PM Matthias J. Sax  wrote:


Thanks for the KIP Shashwat. Closing this testing gap is great! It did
come up a few time already...

One question: why do you propose to `extend MockProcessorContext`?

Given how the actual runtime context classes are setup, it seems that
the regular context and fixed-key-context are distinct, and thus I
believe both mock-context classes should be distinct, too?

What I mean is that FixedKeyProcessorContext does not extend
ProcessorContext. Both classes have a common parent ProcessINGContext
(note the very similar but different names), but they are "siblings"
only, so why make the mock processor a parent-child relationship?

It seems better to do

public class MockFixedKeyProcessorContext
implements FixedKeyProcessorContext,
   RecordCollector.Supplier


Of course, if there is code we can share between both mock-context we
should so this, but it should not leak into the public API?


-Matthias



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

Hi everyone,

I would like to start the discussion on


https://cwiki.apache.org/confluence/display/KAFKA/KIP-1027%3A+Add+MockFixedKeyProcessorContext


This adds MockFixedKeyProcessorContext to the Kafka Streams Test Utils
library.

Regards,
Shashwat Pandey

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

2024-03-28 Thread Matthias J. Sax


Thanks. I think you can start a VOTE.

-Matthias


On 3/20/24 9:24 PM, Lucia Cerchie wrote:

thanks Sophie, I've made the updates, would appreciate one more look before
submission

On Wed, Mar 20, 2024 at 8:36 AM Sophie Blee-Goldman 
wrote:


A few minor notes on the KIP but otherwise I think you can go ahead and
call for a vote!

1. Need to update the Motivation section to say "console clients" or
"console consumer/producer" instead of " plain consumer client"
2. Remove the first paragraph under "Public Interfaces" (ie the KIP-writing
instructions) and also list the new config definitions here. You can just
add a code snippet for each class (TimeWindowedDe/Serializer) with the
actual variable definition you'll be adding. Maybe also add a code snippet
for StreamsConfig with the @Deprecated annotation added to the two configs
we're deprecating
3. nit: remove the "questions" under "Compatibility, Deprecation, and
Migration Plan", ie the stuff from the KIP-writing template. Just makes it
easier to read
4. In "Test Plan" we should also have a unit test to make sure the new "
windowed.inner.de/serializer.class" takes preference in the case that both
it and the old "windowed.inner.serde.class" config are both specified

Also, this is more of a question than a suggestion, but is the KIP title
perhaps a bit misleading in that people might assume these configs will no
longer be available for use at all? What do people think about changing it
to something like

*Move `window.size.ms ` and
`windowed.inner.serde.class` from `StreamsConfig` to
TimeWindowedDe/Serializer class*

A bit long/clunky but at least it gets the point across about where folks
can find these configs going forward.

On Mon, Mar 18, 2024 at 6:26 PM Lucia Cerchie

wrote:


Thanks for the discussion!

I've updated the KIP here with what I believe are the relevant pieces,
please let me know if anything is missing:


https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804


On Sun, Mar 17, 2024 at 7:09 PM Sophie Blee-Goldman <

sop...@responsive.dev



wrote:


Sounds good!

@Lucia when you have a moment can you update the KIP with
the new proposal, including the details that Matthias pointed
out in his last response? After that's done I think you can go
ahead and call for a vote whenever you're ready!

On Sat, Mar 16, 2024 at 7:35 PM Matthias J. Sax 

wrote:



Thanks for the summary. Sounds right to me. That is what I would

propose.


As you pointed out, we of course still need to support the current
confis, and we should log a warning when in use (even if the new one

is

in use IMHO) -- but that's more an implementation detail.

I agree that the new config should take preference in case both are
specified. This should be pointed out in the KIP, as it's an

important

contract the user needs to understand.


-Matthias

On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote:


Should we change it do `.serializer` and `.deserialize`?


That's a good point -- if we're going to split this up by defining

the

config
in both the TimeWindowedSerializer and TimeWindowedDeserializer,
then it makes perfect sense to go a step further and actually

define

only the relevant de/serializer class instead of the full serde

Just to put this all together, it sounds like the proposal is to:

1) Deprecate both these configs where they appear in StreamsConfig
(as per the original plan in the KIP, just reiterating it here)

2) Don't "define" either config in any specific client's Config

class,

but just define a String variable with the config name in the

relevant

de/serializer class, and maybe point people to it in the docs

somewhere


3) We would add three new public String variables for three

different

configs across two classes, specifically:

In TimeWindowedSerializer:
- define a constant for "windowed.inner.serializer.class"
In TimeWindowedDeserializer:
- define a constant for "windowed.inner.deserializer.class"
- define a constant for "window.size.ms"

4) Lastly, we would update the windowed de/serializer

implementations

to check for the new configs (ie "

windowed.inner.de/serializer.class

")

and use the provided de/serializer class, if one was given. If the

new

configs are not present, they would fall back to the

original/current

logic (ie that based on the old "windowed.inner.serde.class"

config)


I think that's everything. Does this sound about right for where we

want

to go with these configs?

On Thu, Mar 14, 2024 at 4:58 PM Matthias J. Sax 

wrote:



By "don't add them" do you just mean we would not have any

actual

variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
and simply document -- somewhere -- that one can use the string
"window.size.ms" when configuring a command-line client with a
windowed serde?


Yes. That's the idea.




I assume you aren't proposing

to remove the ability to use and understand this config from the
implementations themselves, but correct me if

Re: [DISCUSS] KIP-1024: Make the restore behavior of GlobalKTables with custom processors configureable

2024-03-28 Thread Matthias J. Sax

Hey,

looking into the API, I am wondering why we would need to add an
overload talking a `Named` parameter?

StreamsBuilder.addGlobalStore() (and .addGlobalTable()) already takes a
`Consumed` parameter that allows to set a name.

2.
I do not understand what you mean with "maximum flexibility". The built-in processor needs to assume a given state store interface. That means, users have to provide a state store that offers that interface. If they do not they will get a runtime exception. If we require a store builder for a given interface, we can catch the mistake at compile time. Let me know whether I misunderstood something.

Yes, we could catch it at runtime. But I guess what I was trying to say
is different: I was trying to say, we should not limit the API to always
require a specific store, such that global stores can only be of a
certain type. Global Stores should be allowed to be of any type. Hence,
if we add a built-in processor, it can only be one option, and we always
need to support custom processor, and might also want to try to allow
the restore optimization for custom processor (and thus other store
types), not just for our built-in processor (and our built-in stores).
Coupling the optimization to built-in stores would prevent us to apply
the optimization to custom stores.

@Almog: interesting idea. I tend to think that both issues are
orthogonal. If users pick to apply the optimization "added" by this KIP,
the bug you mentioned would still apply to global stores, and thus this
KIP is not addressing the issue you mentioned.

Personally, I also think that we don't need a KIP to fix the ticket you
mentioned? In the end, we need to skip records during restore, and it
seems it does not make sense to make this configurable?

-Matthias

On 3/26/24 4:24 PM, Almog Gavra wrote:

Thanks for the thoughts Bruno!

Do you mean a API to configure restoration instead of boolean flag

reprocessOnRestore?

Yes, this is exactly the type of thing I was musing (but I don't have any
concrete suggestions). It feels like that would give the flexibility to do
things like the motivation section of the KIP (allow bulk loading of
records without reprocessing) while also solving other limitations.

I'm supportive of the KIP as-is but was hoping somebody with more
experience would have a sudden inspiration for how to solve both issues
with one API! Anyway, I'll slide back into the lurking shadows for now and
let the discussion continue :)

Cheers,
Almog

On Tue, Mar 26, 2024 at 4:22 AM Bruno Cadonna wrote:

Hi Almog,

Do you mean a API to configure restoration instead of boolean flag
reprocessOnRestore?

Do you already have an idea?

The proposal in the KIP is focused on the processor that updates the
global state whereas in the case of GlobalKTable and source KTable the
issues lies in the deserialization of records from the input topics, but
only if the deserialization error handler is configured to drop the
problematic record. Additionally, for source KTable the source topic
optimization must be turned on to run into the issue. I am wondering how
a unified API for global stores, GlobalKTable, and source KTable might
look like.

While it is an interesting question, I am in favor of deferring this to
a separate KIP.

Best,
Bruno

On 3/26/24 12:49 AM, Almog Gavra wrote:

Hello Folk!

Glad to see improvements to the GlobalKTables in discussion! I think they
deserve more love :)

Scope creep alert (which I'm generally against and certainly still

support

this KIP without but I want to see if there's an elegant way to address
both problems). The KIP mentions that "Now the restore is done by
reprocessing using an instance from the customer processor supplier"

which

I suppose fixed a long-standing bug (
https://issues.apache.org/jira/browse/KAFKA-8037) but only for
GlobalKTables and not for normal KTables that use the source-changelog
optimization. Since this API could be used to signal "I want to reprocess
on restore" I'm wondering whether it makes sense to design this API in a
way that could be extended for KTables as well so a fix for KAFKA-8037
would be possible with the same mechanism. Thoughts?

Cheers,
Almog

On Mon, Mar 25, 2024 at 11:06 AM Walker Carlson
wrote:

Hey Bruno,

1) I'm actually not sure why that is in there. It certainly doesn't

match

the convention. Best to remove it and match the other methods.

2) Yeah, I thought about it but I'm not convinced it is a necessary
restriction. It might be useful for the already defined processors but

then

they might as well use the `globalTable` method. I think the add state
store option should go for maximum flexibility.

Best,
Walker

On Fri, Mar 22, 2024 at 10:01 AM Bruno Cadonna

wrote:

Hi Walker,

A couple of follow-up questions.

1.
Why do you propose to explicitly pass a parameter "storeName" in
StreamsBuilder#addGlobalStore?
The StoreBuilder should already provide a name for the store, if I
understand the code

Re: [DISCUSS] KIP-950: Tiered Storage Disablement

2024-03-28 Thread Doğuşcan Namal

Hi Satish, I will try to answer as much as I can and the others could chime
in with further details.





*101. For remote.log.disable.policy=delete: Does it delete the remote log
data immediately and the data in remote storage will not be taken into
account by any replica? That means log-start-offset is moved to the earlier
local-log-start-offset.*
*Exactly. RemoteLogData will be deleted immediately. *
*So before the deletion starts we move LogStart offset to
LocalLogStartOffset to ensure that no RemoteLog will be accessed after that
point.*


* 102. Can we update the remote.log.disable.policy after tiered storage is
disabled on a topic?*

*This is a good point. I think we should not allow modifying this
configuration*
*because changing the policy from Deletion to Retain when there is an
ongoing Deletion will result in an undefined behaviour and where we retain
half of the remote log and delete the other half.*

* 103. Do we plan to add any metrics related to this feature?*



*Any recommendations?*
*We may emit a gauge showing the enablement state of a topic but we could
gather that info from the logs as well.*
*The total duration for remote topic deletion could be added as well but
this is more of a metric for the RemotePartitionRemover itself.*



*104. Please add configuration details about copier thread pool, expiration
thread pool and the migration of the existing
RemoteLogManagerScheduledThreadPool.*

*Will add the details.*

105. How is the behaviour with topic or partition deletion request
handled when tiered storage disablement request is still being
processed on a topic?

*If the disablement policy is Delete then a successive topic deletion
request is going to be a NOOP because RemoteLogs is already being deleted.*
*If the disablement policy is Retain, then we only moved the LogStartOffset
and didn't touch RemoteLogs anyway, so the delete topic request will result*

*in the initiation of RemoteLog deletion.*


On Tue, 26 Mar 2024 at 18:21, Kamal Chandraprakash <
kamal.chandraprak...@gmail.com> wrote:

> Hi,
>
> Thanks for the KIP! Overall the KIP looks good and covered most of the
> items.
>
> 1. Could you explain how the brokers will handle the DisableRemoteTopic API
> request?
>
> 2. Who will initiate the controller interaction sequence? Does the
> controller listens for
> topic config updates and initiate the disablement?
>
> --
> Kamal
>
> On Tue, Mar 26, 2024 at 4:40 PM Satish Duggana 
> wrote:
>
> > Thanks Mehari, Divij, Christo etal for the KIP.
> >
> > I had an initial review of the KIP and left the below comments.
> >
> > 101. For remote.log.disable.policy=delete:
> > Does it delete the remote log data immediately and the data in remote
> > storage will not be taken into account by any replica? That means
> > log-start-offset is moved to the earlier local-log-start-offset.
> >
> > 102. Can we update the remote.log.disable.policy after tiered storage
> > is disabled on a topic?
> >
> > 103. Do we plan to add any metrics related to this feature?
> >
> > 104. Please add configuration details about copier thread pool,
> > expiration thread pool and the migration of the existing
> > RemoteLogManagerScheduledThreadPool.
> >
> > 105. How is the behaviour with topic or partition deletion request
> > handled when tiered storage disablement request is still being
> > processed on a topic?
> >
> > ~Satish.
> >
> > On Mon, 25 Mar 2024 at 13:34, Doğuşcan Namal 
> > wrote:
> > >
> > > Hi Christo and Luke,
> > >
> > > I think the KRaft section of the KIP requires slight improvement. The
> > metadata propagation in KRaft is handled by the RAFT layer instead of
> > sending Controller -> Broker RPCs. In fact, KIP-631 deprecated these
> RPCs.
> > >
> > > I will come up with some recommendations on how we could improve that
> > one but until then, @Luke please feel free to review the KIP.
> > >
> > > @Satish, if we want this to make it to Kafka 3.8 I believe we need to
> > aim to get the KIP approved in the following weeks otherwise it will slip
> > and we can not support it in Zookeeper mode.
> > >
> > > I also would like to better understand what is the community's stand
> for
> > adding a new feature for Zookeeper since it is marked as deprecated
> already.
> > >
> > > Thanks.
> > >
> > >
> > >
> > > On Mon, 18 Mar 2024 at 13:42, Christo Lolov 
> > wrote:
> > >>
> > >> Heya,
> > >>
> > >> I do have some time to put into this, but to be honest I am still
> after
> > >> reviews of the KIP itself :)
> > >>
> > >> After the latest changes it ought to be detailing both a Zookeeper
> > approach
> > >> and a KRaft approach.
> > >>
> > >> Do you have any thoughts on how it could be improved or should I
> start a
> > >> voting thread?
> > >>
> > >> Best,
> > >> Christo
> > >>
> > >> On Thu, 14 Mar 2024 at 06:12, Luke Chen  wrote:
> > >>
> > >> > Hi Christo,
> > >> >
> > >> > Any update with this KIP?
> > >> > If you don't have time to complete it, I can collaborate with you to
> > work
> > >> > on it.
> > >> >

[VOTE] 3.6.2 RC2

2024-03-28 Thread Manikumar

Hello Kafka users, developers and client-developers,

This is the second candidate we are considering for the release of Apache
Kafka 3.6.2.

This is a bugfix release with several fixes, including dependency
version bumps for CVEs.

Release notes for the 3.6.2 release:
https://home.apache.org/~manikumar/kafka-3.6.2-rc2/RELEASE_NOTES.html

*** Please download, test and vote by by Wednesday, April 3rd

Kafka's KEYS file containing PGP keys we use to sign the release:
https://kafka.apache.org/KEYS

* Release artifacts to be voted upon (source and binary):
https://home.apache.org/~manikumar/kafka-3.6.2-rc2/


* Maven artifacts to be voted upon:
https://repository.apache.org/content/groups/staging/org/apache/kafka/

* Javadoc:
https://home.apache.org/~manikumar/kafka-3.6.2-rc2/javadoc/

* Tag to be voted upon (off 3.6 branch) is the 3.6.2 tag:
https://github.com/apache/kafka/releases/tag/3.6.2-rc2

* Documentation:
https://kafka.apache.org/36/documentation.html

* Protocol:
https://kafka.apache.org/36/protocol.html

* Successful Jenkins builds for the 3.6 branch:
Unit/integration tests:
https://ci-builds.apache.org/job/Kafka/job/kafka/job/3.6/167/ (test
failures passed in local runs)
System tests: I will update system test results soon.


Thanks,
Manikumar

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

2024-03-28 Thread Andrew Schofield

Hi David,
I agree that we should use the same mechanism to gate KIP-932 once that
feature reaches production readiness. The precise details of the values will
depend upon the current state of all these flags when that release comes.

Thanks,
Andrew

> On 28 Mar 2024, at 07:11, David Jacot  wrote:
> 
> Hi, Jun, Justine,
> 
> Regarding `group.coordinator.version`, the idea is to use it to gate
> records and APIs of the group coordinator. The first use case will be
> KIP-848. We will use version 2 of the flag to gate all the new records and
> the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So version
> 1 will be the only the old protocol and version 2 will be the currently
> implemented new protocol. I don't think that we have any dependency on the
> metadata version at the moment. The changes are orthogonal. I think that we
> could mention KIP-848 as the first usage of this flag in the KIP. I will
> also update KIP-848 to include it when this KIP is accepted. Another use
> case is the Queues KIP. I think that we should also use this new flag to
> gate it.
> 
> Best,
> David
> 
> On Thu, Mar 28, 2024 at 1:14 AM Jun Rao  wrote:
> 
>> Hi, Justine,
>> 
>> Thanks for the reply.
>> 
>> So, "dependencies" and "version-mapping" will be added to both
>> kafka-feature and kafka-storage? Could we document that in the tool format
>> section?
>> 
>> Jun
>> 
>> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan
>> 
>> wrote:
>> 
>>> Ok. I can remove the info from the describe output.
>>> 
>>> Dependencies is needed for the storage tool because we want to make sure
>>> the desired versions we are setting will be valid. Version mapping should
>>> be for both tools since we have --release-version for both tools.
>>> 
>>> I was considering changing the IV strings, but I wasn't sure if there
>> would
>>> be some disagreement with the decision. Not sure if that breaks
>>> compatibility etc. Happy to hear everyone's thoughts.
>>> 
>>> Justine
>>> 
>>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao 
>> wrote:
>>> 
 Hi, Justine,
 
 Thanks for the reply.
 
 Having "kafka-feature dependencies" seems enough to me. We don't need
>> to
 include the dependencies in the output of "kafka-feature describe".
 
 We only support "dependencies" in kafka-feature, not kafka-storage. We
 probably should do the same for "version-mapping".
 
 bin/kafka-features.sh downgrade --feature metadata.version=16
 --transaction.protocol.version=2
 We need to add the --feature flag for the second feature, right?
 
 In "kafka-features.sh describe", we only show the IV string for
 metadata.version. Should we also show the level number?
 
 Thanks,
 
 Jun
 
 On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan
 
 wrote:
 
> I had already included this example
> bin/kafka-features.sh downgrade --feature metadata.version=16
> --transaction.protocol.version=2 // Throws error if metadata version
>>> is <
> 16, and this would be an upgrade
> But I have updated the KIP to explicitly say the text you mentioned.
> 
> Justine
> 
> On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio
>  wrote:
> 
>> Hi Justine,
>> 
>> See my comment below.
>> 
>> On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan
>>  wrote:
>>> The feature command includes the upgrade or downgrade command
>> along
> with
>>> the --release-version flag. If some features are not moving in
>> the
>>> direction mentioned (upgrade or downgrade) the command will fail
>> --
>> perhaps
>>> with an error of which features were going in the wrong
>> direction.
>> 
>> How about updating the KIP to show and document this behavior?
>> 
>> Thanks,
>> --
>> -José
>> 
> 
 
>>> 
>>

Jenkins build is still unstable: Kafka » Kafka Branch Builder » 3.6 #167