Build failed in Jenkins: Kafka » kafka-trunk-jdk11 #91

[Apache Jenkins Server]

See 


Changes:

[github] MINOR: standardize rebalance related logging for easy discovery & 
debugging (#9295)


--
[...truncated 3.35 MB...]
org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPassRecordHeadersIntoSerializersAndDeserializers[Eos enabled = false] 
PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessConsumerRecordList[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessConsumerRecordList[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUseSinkSpecificSerializers[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUseSinkSpecificSerializers[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldFlushStoreForFirstInput[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldFlushStoreForFirstInput[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessFromSourceThatMatchPattern[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessFromSourceThatMatchPattern[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureSinkTopicNamesIfWrittenInto[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureSinkTopicNamesIfWrittenInto[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUpdateStoreForNewKey[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUpdateStoreForNewKey[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldSendRecordViaCorrectSourceTopicDeprecated[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldSendRecordViaCorrectSourceTopicDeprecated[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTime[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTime[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldSetRecordMetadata[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldSetRecordMetadata[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForLargerValue[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForLargerValue[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessRecordForTopic[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessRecordForTopic[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldForwardRecordsFromSubtopologyToSubtopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldForwardRecordsFromSubtopologyToSubtopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForSmallerValue[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForSmallerValue[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCreateStateDirectoryForStatefulTopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCreateStateDirectoryForStatefulTopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateIfWallClockTimeAdvances[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateIfWallClockTimeAdvances[Eos enabled = false] PASSED

org.apache.kafka.streams.test.TestRecordTest > testConsumerRecord STARTED

org.apache.kafka.streams.test.TestRecordTest > testConsumerRecord 

Jenkins build is back to normal : Kafka » kafka-trunk-jdk15 #90

[Apache Jenkins Server]

See 

Build failed in Jenkins: Kafka » kafka-trunk-jdk11 #90

[Apache Jenkins Server]

See 


Changes:

[github] KAFKA-10077: Filter downstream of state-store results in spurious 
tombstones (#9156)


--
[...truncated 6.70 MB...]

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessConsumerRecordList[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUseSinkSpecificSerializers[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUseSinkSpecificSerializers[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldFlushStoreForFirstInput[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldFlushStoreForFirstInput[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessFromSourceThatMatchPattern[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessFromSourceThatMatchPattern[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureSinkTopicNamesIfWrittenInto[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureSinkTopicNamesIfWrittenInto[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUpdateStoreForNewKey[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUpdateStoreForNewKey[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldSendRecordViaCorrectSourceTopicDeprecated[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldSendRecordViaCorrectSourceTopicDeprecated[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTime[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTime[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldSetRecordMetadata[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldSetRecordMetadata[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForLargerValue[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForLargerValue[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessRecordForTopic[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessRecordForTopic[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldForwardRecordsFromSubtopologyToSubtopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldForwardRecordsFromSubtopologyToSubtopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForSmallerValue[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForSmallerValue[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCreateStateDirectoryForStatefulTopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCreateStateDirectoryForStatefulTopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateIfWallClockTimeAdvances[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateIfWallClockTimeAdvances[Eos enabled = false] PASSED

org.apache.kafka.streams.test.TestRecordTest > testConsumerRecord STARTED

org.apache.kafka.streams.test.TestRecordTest > testConsumerRecord PASSED

org.apache.kafka.streams.test.TestRecordTest > testToString STARTED

org.apache.kafka.streams.test.TestRecordTest > testToString PASSED

org.apache.kafka.streams.test.TestRecordTest > testInvalidRecords STARTED

Re: [VOTE] KIP-584: Versioning scheme for features

[Kowshik Prakasam]

Hi Colin,

Thanks for the feedback. Those are very good points. I have made the
following changes to the KIP as you had suggested:
1. Included the `timeoutMs` field in the `UpdateFeaturesRequest` schema.
The initial implementation won't be making use of the field, but we can
always use it in the future as the need arises.
2. Modified the `FinalizedFeaturesEpoch` field in `ApiVersionsResponse` to
use int64. This is to avoid overflow problems in the future once ZK is gone.

I have also incorporated these changes into the versioning write path PR
that is currently under review: https://github.com/apache/kafka/pull/9001.


Cheers,
Kowshik



On Fri, Sep 25, 2020 at 4:57 PM Kowshik Prakasam 
wrote:

> Hi Jun,
>
> Thanks for the feedback. It's a very good point. I have now modified the
> KIP-584 write-up "goals" section a bit. It now mentions one of the goals as
> enabling rolling upgrades using a single restart (instead of 2). Also I
> have removed the text explicitly aiming for deprecation of IBP. Note that
> previously under "Potential features in Kafka" the IBP was mentioned under
> point (4) as a possible coarse-grained feature. Hopefully, now the 2
> sections of the KIP align with each other well.
>
>
> Cheers,
> Kowshik
>
>
> On Fri, Sep 25, 2020 at 2:03 PM Colin McCabe  wrote:
>
>> On Tue, Sep 22, 2020, at 00:43, Kowshik Prakasam wrote:
>> > Hi all,
>> >
>> > I wanted to let you know that I have made the following changes to the
>> > KIP-584 write up. The purpose is to ensure the design is correct for a
>> few
>> > things which came up during implementation:
>> >
>>
>> Hi Kowshik,
>>
>> Thanks for the updates.
>>
>> >
>> > 1. Per FeatureUpdate error code: The UPDATE_FEATURES controller API is
>> no
>> > longer transactional. Going forward, we allow for individual
>> FeatureUpdate
>> > to succeed/fail in the request. As a result, the response schema now
>> > contains an error code per FeatureUpdate as well as a top-level error
>> code.
>> > Overall this is a better design because it better represents the nature
>> of
>> > the API: each FeatureUpdate in the request is independent of the other
>> > updates, and the controller can process/apply these independently to ZK.
>> > When an UPDATE_FEATURES request fails, this new design provides better
>> > clarity to the caller on which FeatureUpdate could not be applied (via
>> the
>> > individual error codes). In the previous design, we were unable to
>> achieve
>> > such an increased level of clarity in communicating the error codes.
>> >
>>
>> OK
>>
>> >
>> > 2. Due to #1, there were some minor changes required to the proposed
>> Admin
>> > APIs (describeFeatures and updateFeatures). A few unnecessary public
>> APIs
>> > have been removed, and couple essential ones have been added. The latest
>> > changes now represent the latest design.
>> >
>> > 3. The timeoutMs field has been removed from the the UPDATE_FEATURES API
>> > request, since it was not found to be required during implementation.
>> >
>>
>> Please don't get rid of timeoutMs.  timeoutMs is required if you want to
>> implement the ability to timeout the call if the controller can't get to it
>> in time.  This is important for avoiding congestion collapse where the
>> controller collapses under the weight of lots of retries of the same set of
>> calls.
>>
>> We may not be able to do it in the initial implementation, but we will
>> eventually implement this for all the controller-bound RPCs.
>>
>> > >
>> > > 2. Finalized feature version epoch data type has been made to be int32
>> > > (instead of int64). The reason is that the epoch value is the value
>> of ZK
>> > > node version, whose data type is int32.
>> > >
>>
>> Sorry, I missed this earlier.  Using 16 bit feature levels seems fine.
>> However, please don't use a 32-bit epoch here.  We deliberately made the
>> epoch 64 bits to avoid overflow problems in the future once ZK is gone.
>>
>> best,
>> Colin
>>
>> > > 3. Introduced a new 'status' field in the '/features' ZK node schema.
>> The
>> > > purpose is to implement Colin's earlier point for the strategy for
>> > > transitioning from not having a /features znode to having one. An
>> > > explanation has been provided in the following section of the KIP
>> detailing
>> > > the different cases:
>> > >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP584:Versioningschemeforfeatures-FeatureZKnodestatus
>> > > .
>> > >
>> > > Please let me know if you have any questions or concerns.
>> > >
>> > >
>> > > Cheers,
>> > > Kowshik
>> > >
>> > >
>> > >
>> > > Cheers,
>> > > Kowshik
>> > >
>> > > On Tue, Apr 28, 2020 at 11:24 PM Kowshik Prakasam <
>> kpraka...@confluent.io>
>> > > wrote:
>> > >
>> > >> Hi all,
>> > >>
>> > >> This KIP vote has been open for ~12 days. The summary of the votes is
>> > >> that we have 3 binding votes (Colin, Guozhang, Jun), and 3
>> non-binding
>> > >> votes (David, Dhruvil, Boyang). Therefore, the KIP vote passes. I'll
>> mark

Re: [VOTE] KIP-590: Redirect Zookeeper Mutation Protocols to The Controller

[Jason Gustafson]

Hey All,

So the main thing the EnvelopeRequest gives us is a way to avoid converting
older API versions in order to attach the initial principal name and the
clientId. It also saves the need to add the initial principal and client id
as a tagged field to all of the forwarded protocols, which is nice. We
still have the challenge of advertising API versions which are compatible
with both the broker receiving the request and the controller that the
request is ultimately forwarded to, but not sure I see a way around that.

I realize I might be walking into a minefield here, but since the envelope
is being revisited, it seems useful to compare the approach suggested above
with the option relying on impersonation. I favor the use of impersonation
because it makes forwarding simpler. As the proposal stands, we will have
to maintain logic for each forwarded API to unpack, authorize, and repack
any forwarded requests which flow through the broker. This is probably not
a huge concern from an efficiency perspective as long as we are talking
about just the Admin APIs, but it does have a big maintenance cost since
we'll need to ensure that every new field gets properly carried through. It
would be nice if we just didn't have to think about that. We also might
eventually come up with reasons to extend forwarding to non-admin APIs, so
it would be nice to start with an efficient approach.

It seems like the main difference comes down to where the authorization is
done. Suppose that broker B receives an AlterConfig request from the client
in order to change topic configs and wants to forward to controller C.

Option 1 (no impersonation): B authorizes AlterConfigs for the included
topics with the client principal. Rejected topics are stripped out of the
request.  Authorized topics are repackaged into a new request and sent in
an envelope to C, which verifies ClusterAction permission with the broker
principal and assumes authorization for the underlying request
Option 2 (with impersonation): B authenticates the client, but does no
authorization and forwards the request in an envelope to C containing the
authenticated principal. C checks ClusterAction for the envelope request
using the broker principal and AlterConfigs for the underlying request
using the forwarded client principal.

In either case, broker B implicitly gets AlterConfigs permission for the
topic. This is true even without the envelope and seems like a reasonable
requirement. The broker should itself be authorized to perform any action
that it might have to forward requests for. As far as I know, all the
proposals we have considered require this. The main question from a
security perspective is whether any of these proposals require additional
unnecessary access, which is probably the main doubt about impersonation.
However, there are a couple ways we can restrict it:

1. We can restrict the principals that are allowed to be impersonated
2. We can restrict the actions that are possible through impersonation.

Considering the first point, there's probably no reason to allow
impersonation of superusers. Additionally, a custom authorizer could forbid
impersonation outside of a particular group. To support this, it would be
helpful to extend `KafkaPrincipal` or `AuthorizableRequestContext` so that
it indicates whether a request is an impersonated request.

Considering the second point, it doesn't make sense to allow arbitrary
requests to be forwarded. We know exactly the set of forwardable APIs and
we can reject any other APIs without even looking at the principal. This is
the nice thing that the Envelope request gives us. I don't know if we would
ever have finer-grained restrictions, but in principle I don't see why we
couldn't.

In the future, I guess we could go even further so that the broker itself
wouldn't need the same permissions as the client. If the client and the
controller shared some secret or if the client had a public key that we
could rely on, then the client could send along a MAC or token of some
sort, which could then be forwarded through the envelope. Then the broker
would not be allowed to do anything except exactly what the client
requested. I'm not suggesting we do this, just that we will have the
flexibility for it.

>From the discussion thread, it looks like the main problem here is that
`KafkaPrincipal` does not currently have a defined serialization mechanism.
We can add this, but it is a breaking change, so we have to wait for a
major release version before we make it a requirement. This is fine from
the perspective of KIP-500 since the bridge release will require a major
release bump anyway. What I imagine we could do is something like this:

1. In versions 2.7/2.8/.., we can add the new API in a mixin interface, say
`KafkaPrincipalSerde`. We can implement this interface for
`DefaultKafkaPrincipalBuilder` trivially. On startup, we can check if this
interface is implemented by the provided `KafkaPrincipalBuilder`. If it is
not, we can 

Re: [DISCUSS] KIP-631: The Quorum-based Kafka Controller

[Jun Rao]

Hi, Colin,

Thanks for the reply.

62. Thinking about this more, I am wondering what's our overall strategy
for configs shared between the broker and the controller. For example, both
the broker and the controller have to define listeners. So both need
configs like listeners/advertised.listeners. Both the new controller and
the broker replicate data. So both need to define some replication related
configurations (replica.fetch.min.bytes, replica.fetch.wait.max.ms, etc).
Should we let the new controller share those configs with brokers or should
we define new configs just for the controller? It seems that we want to
apply the same strategy consistently for all shared configs?

63. If a node has process.roles set to controller, does one still need to
set broker.id on this node?

Thanks,

Jun



On Fri, Sep 25, 2020 at 2:17 PM Colin McCabe  wrote:

> On Fri, Sep 25, 2020, at 10:17, Jun Rao wrote:
> > Hi, Colin,
> >
> > Thanks for the reply.
> >
> > 60. Yes, I think you are right. We probably need the controller id when a
> > broker starts up. A broker only stores the Raft leader id in the metadata
> > file. To do the initial fetch to the Raft leader, it needs to know the
> > host/port associated with the leader id.
> >
> > 62. It seems there are 2 parts to this : (1) which listener a client
> should
> > use to initiate a connection to the controller and (2) which listener
> > should a controller use to accept client requests. For (1), at any point
> of
> > time, a client only needs to use one listener. I think
> > controller.listener.name is meant for the client.
>
> Hi Jun,
>
> controller.listener.names is also used by the controllers.  In the case
> where we have a broker and a controller in the same JVM, we have a single
> config file.  Then we need to know which listeners belong to the controller
> and which belong to the broker component.  That's why it's a list.
>
> > So, a single value seems
> > to make more sense. Currently, we don't have a configuration for (2). We
> > could add a new one for that and support a list. I am wondering how
> useful
> > it will be. One example that I can think of is that we can reject
> > non-controller related requests if accepted on controller-only listeners.
> > However, we already support separate authentication for the controller
> > listener. So, not sure how useful it is.
>
> The controller always has a separate listener and does not share listeners
> with the broker.  The main reason for this is to avoid giving clients the
> ability to launch a denial-of-service attack on the controller by flooding
> a broker port.  A lot of times, these attacks are made unintentionally by
> poorly coded or configured clients.  Additionally, broker ports can also be
> very busy with large fetch requests, and so on.  It's just a bad
> configuration in general to try to overload the same port for the
> controller and broker, and we don't want to allow it.  It would be a
> regression to go from the current system where control requests are safely
> isolated on a separate port, to one where they're not.  It also makes the
> code and configuration a lot messier.
>
> >
> > 63. (a) I think most users won't know controller.id defaults to
> broker.id +
> > 3000. So, it can be confusing for them to set up controller.connect. If
> > this is truly needed, it seems that it's less confusing to make
> > controller.id required.
> > (b) I am still trying to understand if we truly need to expose a
> > controller.id. What if we only expose broker.id and let
> controller.connect
> > just use broker.id? What will be missing?
>
> The controller has a separate ID from the broker, so knowing broker.id is
> not helpful here.
>
> best,
> Colin
>
> >
> > Thanks,
> >
> > Jun
> >
> > On Thu, Sep 24, 2020 at 10:55 PM Colin McCabe 
> wrote:
> >
> > > On Thu, Sep 24, 2020, at 16:24, Jun Rao wrote:
> > > > Hi, Colin,
> > > >
> > > > Thanks for the reply and the updated KIP. A few more comments below.
> > > >
> > >
> > > Hi Jun,
> > >
> > > >
> > > > 53. It seems that you already incorporated the changes in KIP-516.
> With
> > > > topic ids, we don't need to wait for the topic's data to be deleted
> > > before
> > > > removing the topic metadata. If the topic is recreated, we can still
> > > delete
> > > > the data properly based on the topic id. So, it seems that we can
> remove
> > > > TopicRecord.Deleting.
> > > >
> > >
> > > Thanks for the reply.  What I was thinking of doing here was using
> topic
> > > IDs internally, but still using names externally.  So the topic UUIDs
> are
> > > only for the purpose of associating topics with partitions -- from the
> > > user's point of view, topics are still identified by names.
> > >
> > > You're right that KIP-516 will simplify things, but I'm not sure when
> that
> > > will land, so I wanted to avoid blocking the initial implementation of
> this
> > > KIP on it.
> > >
> > > >
> > > > 55. It seems to me that the current behavior where we favor the
> current
> > > > 

Re: [VOTE] KIP-584: Versioning scheme for features

[Kowshik Prakasam]

Hi Jun,

Thanks for the feedback. It's a very good point. I have now modified the
KIP-584 write-up "goals" section a bit. It now mentions one of the goals as
enabling rolling upgrades using a single restart (instead of 2). Also I
have removed the text explicitly aiming for deprecation of IBP. Note that
previously under "Potential features in Kafka" the IBP was mentioned under
point (4) as a possible coarse-grained feature. Hopefully, now the 2
sections of the KIP align with each other well.


Cheers,
Kowshik


On Fri, Sep 25, 2020 at 2:03 PM Colin McCabe  wrote:

> On Tue, Sep 22, 2020, at 00:43, Kowshik Prakasam wrote:
> > Hi all,
> >
> > I wanted to let you know that I have made the following changes to the
> > KIP-584 write up. The purpose is to ensure the design is correct for a
> few
> > things which came up during implementation:
> >
>
> Hi Kowshik,
>
> Thanks for the updates.
>
> >
> > 1. Per FeatureUpdate error code: The UPDATE_FEATURES controller API is no
> > longer transactional. Going forward, we allow for individual
> FeatureUpdate
> > to succeed/fail in the request. As a result, the response schema now
> > contains an error code per FeatureUpdate as well as a top-level error
> code.
> > Overall this is a better design because it better represents the nature
> of
> > the API: each FeatureUpdate in the request is independent of the other
> > updates, and the controller can process/apply these independently to ZK.
> > When an UPDATE_FEATURES request fails, this new design provides better
> > clarity to the caller on which FeatureUpdate could not be applied (via
> the
> > individual error codes). In the previous design, we were unable to
> achieve
> > such an increased level of clarity in communicating the error codes.
> >
>
> OK
>
> >
> > 2. Due to #1, there were some minor changes required to the proposed
> Admin
> > APIs (describeFeatures and updateFeatures). A few unnecessary public APIs
> > have been removed, and couple essential ones have been added. The latest
> > changes now represent the latest design.
> >
> > 3. The timeoutMs field has been removed from the the UPDATE_FEATURES API
> > request, since it was not found to be required during implementation.
> >
>
> Please don't get rid of timeoutMs.  timeoutMs is required if you want to
> implement the ability to timeout the call if the controller can't get to it
> in time.  This is important for avoiding congestion collapse where the
> controller collapses under the weight of lots of retries of the same set of
> calls.
>
> We may not be able to do it in the initial implementation, but we will
> eventually implement this for all the controller-bound RPCs.
>
> > >
> > > 2. Finalized feature version epoch data type has been made to be int32
> > > (instead of int64). The reason is that the epoch value is the value of
> ZK
> > > node version, whose data type is int32.
> > >
>
> Sorry, I missed this earlier.  Using 16 bit feature levels seems fine.
> However, please don't use a 32-bit epoch here.  We deliberately made the
> epoch 64 bits to avoid overflow problems in the future once ZK is gone.
>
> best,
> Colin
>
> > > 3. Introduced a new 'status' field in the '/features' ZK node schema.
> The
> > > purpose is to implement Colin's earlier point for the strategy for
> > > transitioning from not having a /features znode to having one. An
> > > explanation has been provided in the following section of the KIP
> detailing
> > > the different cases:
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP584:Versioningschemeforfeatures-FeatureZKnodestatus
> > > .
> > >
> > > Please let me know if you have any questions or concerns.
> > >
> > >
> > > Cheers,
> > > Kowshik
> > >
> > >
> > >
> > > Cheers,
> > > Kowshik
> > >
> > > On Tue, Apr 28, 2020 at 11:24 PM Kowshik Prakasam <
> kpraka...@confluent.io>
> > > wrote:
> > >
> > >> Hi all,
> > >>
> > >> This KIP vote has been open for ~12 days. The summary of the votes is
> > >> that we have 3 binding votes (Colin, Guozhang, Jun), and 3 non-binding
> > >> votes (David, Dhruvil, Boyang). Therefore, the KIP vote passes. I'll
> mark
> > >> KIP as accepted and start working on the implementation.
> > >>
> > >> Thanks a lot!
> > >>
> > >>
> > >> Cheers,
> > >> Kowshik
> > >>
> > >> On Mon, Apr 27, 2020 at 12:15 PM Colin McCabe 
> wrote:
> > >>
> > >>> Thanks, Kowshik.  +1 (binding)
> > >>>
> > >>> best,
> > >>> Colin
> > >>>
> > >>> On Sat, Apr 25, 2020, at 13:20, Kowshik Prakasam wrote:
> > >>> > Hi Colin,
> > >>> >
> > >>> > Thanks for the explanation! I agree with you, and I have updated
> the
> > >>> > KIP.
> > >>> > Here is a link to relevant section:
> > >>> >
> > >>>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Controller:ZKnodebootstrapwithdefaultvalues
> > >>> >
> > >>> >
> > >>> > Cheers,
> > >>> > Kowshik
> > >>> >
> > >>> > On Fri, Apr 24, 2020 at 8:50 PM Colin 

Re: [DISCUSS] KIP-630: Kafka Raft Snapshot

[Jun Rao]

Hi, Jose,

Thanks for the reply. A few more comments below.

20. Good point on metadata cache. I think we need to make a decision
consistently. For example, if we decide that dedicated voter nodes don't
serve metadata requests, then we don't need to expose the voters host/port
to the client. Which KIP should make this decision?

31. controller.snapshot.minimum.records: For a compacted topic, we use a
ratio instead of the number of records to determine when to compact. This
has some advantages. For example, if we use
controller.snapshot.minimum.records and set it to 1000, then it will
trigger the generation of a new snapshot when the existing snapshot is
either 10MB or 1GB. Intuitively, the larger the snapshot, the more
expensive it is to write to disk. So, we want to wait for more data to be
accumulated before generating the next snapshot. The ratio based setting
achieves this. For instance, a 50% ratio requires 10MB/1GB more data to be
accumulated to regenerate a 10MB/1GB snapshot respectively.

32. max.replication.lag.ms: It seems this is specific to the metadata
topic. Could we make that clear in the name?

Thanks,

Jun

On Fri, Sep 25, 2020 at 12:43 PM Jose Garcia Sancio 
wrote:

> Thanks for the detailed feedback Jun.
>
> The changes are here:
>
> https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=158864763=25=24
>
> Here is a summary of the change to the KIP:
> 1. Use end offset for snapshot and snapshot id.
> 2. Include default for all of the new configuration options.
> 3. Provide more detail in the response handling for FetchSnapshot
>
> > 20. "Metadata Cache: The component that generates snapshots, reads
> > snapshots and reads logs for observer replicas of the topic partition
> > __cluster_metadata." It seems this is needed on every broker, not just
> > observers?
>
> Yes. I think we need some clarification and consensus here. Some
> people are advocating for Kafka brokers to only be observers and would
> only contain a Metadata Cache. With the Kafka Controllers being
> separate nodes that are voters (follower, candidate or leader) and not
> observers. Others are advocating for Kafka Brokers to be able to host
> both the Kafka Controller and the Metadata Cache. In this case if the
> Controller and Metadata Cache are sharing the same underlying topic
> partition then we need to make sure that we unify the snapshotting
> logic.
>
> I would like to be able to unify the in-memory state for both the
> Kafka Controller and the Metadata Cache so that we can share the same
> replicated log and snapshot.
>
> > 21. Our current convention is to use exclusive offset for naming
> > checkpoint files. For example, a producer snapshot file of 1234.snapshot
> > means that the file includes the producer state up to, but not including
> > offset 1234. So, we probably want to follow the same convention for the
> new
> > checkpoint file.
>
> Thanks for pointing this out. This sounds good to me. This was a
> detail that I was struggling with when reading the replication code.
> Updated the KIP. Wherever the offset is exclusive, I renamed it to
> "end offset" (EndOffset).
>
> > 22. Snapshot Format: KIP-631 only defines the format for individual
> > records. It seems that we need to define the container format here. For
> > example, we need to store the length of each record. Also, does the
> > snapshot file need a CRC field?
>
> Yes. I have added more information on this. In summary, we are going
> to use Kafka's log format version 2. This will give us support for
> compression and CRC at the record batch level. The Kafka Controller
> and Metadata Cache can control how big they want the batches to be.
>
> > 23. Could we provide the default value for the new
> > configs controller.snapshot.minimum.records and max.replication.lag.ms.
> > Also, max.replication.lag.ms seems to just control the snapshot
> frequency
> > by time and not directly relate to replication. So, maybe it should be
> > called sth like controller.snapshot.minimum.interval.ms?
>
> "max.replication.lag.ms" is very similar to "replica.lag.time.max.ms".
> Kafka uses "replica.lag.time.max.ms" to make progress on the
> high-watermark when replicas are slow or offline. We want to use
> "max.replication.lag.ms" to make progress on the LBO when replicas are
> slow or offline. These very similar names are confusing. How about
> "replica.lbo.lag.time.max.ms"?
>
> How often snapshotting will happen is determined by
> "controller.snapshot.minimum.records".
>
> > 24. "Kafka allows the clients to delete records that are less than a
> given
> > offset by using the DeleteRecords RPC . Those requests will be validated
> > using the same logic enumerated above." Hmm, should we allow deleteRecord
> > on the metadata topic? If we do, does it trim the snapshot accordingly?
>
> Yeah. After thinking about it some more, I don't think we shouldn't
> allow DeleteRecords to succeed on the __cluster_metadata partition.
> For the error that we return it 

Re: [DISCUSS] KIP-671: Shutdown Streams Application when appropriate exception is thrown

[Matthias J. Sax]

I am wondering about the usage pattern of this new method.

As already discussed, the method only works if there is at least one
running thread... Do we have any sense how many apps actually run
multi-threaded vs single-threaded? It seems that the feature might be
quite limited without having a handler that is called _before_ the
thread dies? However, for this case, I am wondering if it might be
easier to just return a enum type from such a handler instead of calling
`KakfaStreams#initiateClosingAllClients()`?

In general, it seems that there is some gap between the case of stopping
all instances from "outside" (as proposed in the KIP), vs from "inside"
(what I though was the original line of thinking for this KIP?).

For the network partitioning case, should we at least shutdown all local
threads? It might be sufficient that only one thread sends the "shutdown
signal" while all others just shut down directly? Why should the other
thread wait for shutdown signal for a rebalance? Or should we recommend
to call `initiateClosingAllClients()` followed to `close()` to make sure
that at least the local threads stop (what might be a little bit odd)?

-Matthias

On 9/24/20 7:51 AM, John Roesler wrote:
> Hello all,
> 
> Thanks for bringing this up, Bruno. It’s a really good point that a 
> disconnected node would miss the signal and then resurrect a single-node 
> “zombie cluster” when it reconnects.
> 
> Offhand, I can’t think of a simple and reliable way to distinguish this case 
> from one in which an operator starts a node manually after a prior shutdown 
> signal. Can you? Right now, I’m inclined to agree with Walker that we should 
> leave this as a problem for the future. 
> 
> It should certainly be mentioned in the kip, and it also deserves special 
> mention in our javadoc and html docs for this feature. 
> 
> Thanks!
> John
> 
> On Wed, Sep 23, 2020, at 17:49, Walker Carlson wrote:
>> Bruno,
>>
>> I think that we can't guarantee that the message will get
>> propagated perfectly in every case of, say network partitioning, though it
>> will work for many cases. So I would say it's best effort and I will
>> mention it in the kip.
>>
>> As for when to use it I think we can discuss if this will be
>> sufficient when we come to it, as long as we document its capabilities.
>>
>> I hope this answers your question,
>>
>> Walker
>>
>> On Tue, Sep 22, 2020 at 12:33 AM Bruno Cadonna  wrote:
>>
>>> Walker,
>>>
>>> I am sorry, but I still have a comment on the KIP although you have
>>> already started voting.
>>>
>>> What happens when a consumer of the group skips the rebalancing that
>>> propagates the shutdown request? Do you give a guarantee that all Kafka
>>> Streams clients are shutdown or is it best effort? If it is best effort,
>>> I guess the proposed method might not be used in critical cases where
>>> stopping record consumption may prevent or limit damage. I am not saying
>>> that it must be a guarantee, but this question should be answered in the
>>> KIP, IMO.
>>>
>>> Best,
>>> Bruno
>>>
>>> On 22.09.20 01:14, Walker Carlson wrote:
 The error code right now is the assignor error, 2 is coded for shutdown
 but it could be expanded to encode the causes or for other errors that
>>> need
 to be communicated. For example we can add error code 3 to close the
>>> thread
 but leave the client in an error state if we choose to do so in the
>>> future.

 On Mon, Sep 21, 2020 at 3:43 PM Boyang Chen 
 wrote:

> Thanks for the KIP Walker.
>
> In the KIP we mentioned "In order to communicate the shutdown request
>>> from
> one client to the others we propose to update the SubcriptionInfoData to
> include a short field which will encode an error code.", is there a
> dedicated error code that we should define here, or it is case-by-case?
>
> On Mon, Sep 21, 2020 at 1:38 PM Walker Carlson 
> wrote:
>
>> I am changing the name to "Add method to Shutdown entire Streams
>> Application" since we are no longer using an Exception, it seems more
>> appropriate.
>>
>> Also it looks like the discussion is pretty much finished so I will be
>> calling it to a vote.
>>
>> thanks,
>> Walker
>>
>> On Sat, Sep 19, 2020 at 7:49 PM Guozhang Wang 
> wrote:
>>
>>> Sounds good to me. I also feel that this call should be non-blocking
> but
>> I
>>> guess I was confused from the discussion thread that the API is
> designed
>> in
>>> a blocking fashion which contradicts with my perspective and hence I
>> asked
>>> for clarification :)
>>>
>>> Guozhang
>>>
>>>
>>> On Wed, Sep 16, 2020 at 12:32 PM Walker Carlson <
>>> wcarl...@confluent.io
>>
>>> wrote:
>>>
 Hello Guozhang,

 As for the logging I plan on having three logs. First, the client log
>>> that
 it is requesting an application shutdown, second, the leader log

Build failed in Jenkins: Kafka » kafka-trunk-jdk15 #89

[Apache Jenkins Server]

See 


Changes:

[github] KAFKA-10519; Add missing unit test for `VotedState` (#9337)

[github] MINOR: Use the automated protocol for the Consumer Protocol's 
subscriptions and assignments (#8897)


--
[...truncated 3.35 MB...]
org.apache.kafka.streams.test.OutputVerifierTest > 
shouldPassIfKeyAndValueAndTimestampIsEqualWithNullForCompareKeyValueTimestampWithProducerRecord
 PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullForCompareKeyValueWithProducerRecord STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullForCompareKeyValueWithProducerRecord PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentForCompareKeyValueTimestampWithProducerRecord 
STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentForCompareKeyValueTimestampWithProducerRecord PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldNotAllowNullProducerRecordWithExpectedRecordForCompareValueTimestamp 
STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldNotAllowNullProducerRecordWithExpectedRecordForCompareValueTimestamp 
PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldNotAllowNullExpectedRecordForCompareValue STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldNotAllowNullExpectedRecordForCompareValue PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullReversForCompareKeyValueTimestampWithProducerRecord
 STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullReversForCompareKeyValueTimestampWithProducerRecord
 PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldNotAllowNullProducerRecordForCompareKeyValue STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldNotAllowNullProducerRecordForCompareKeyValue PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullReversForCompareKeyValueWithProducerRecord 
STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullReversForCompareKeyValueWithProducerRecord 
PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldPassIfKeyAndValueAndTimestampIsEqualForCompareKeyValueTimestamp STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldPassIfKeyAndValueAndTimestampIsEqualForCompareKeyValueTimestamp PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullForCompareKeyValueTimestampWithProducerRecord
 STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullForCompareKeyValueTimestampWithProducerRecord
 PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfKeyIsDifferentWithNullReversForCompareKeyValueWithProducerRecord 
STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfKeyIsDifferentWithNullReversForCompareKeyValueWithProducerRecord 
PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldPassIfKeyAndValueIsEqualWithNullForCompareKeyValueWithProducerRecord 
STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldPassIfKeyAndValueIsEqualWithNullForCompareKeyValueWithProducerRecord 
PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfKeyIsDifferentWithNullForCompareKeyValueTimestampWithProducerRecord 
STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfKeyIsDifferentWithNullForCompareKeyValueTimestampWithProducerRecord 
PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentForCompareKeyValueTimestamp STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentForCompareKeyValueTimestamp PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldPassIfValueAndTimestampIsEqualForCompareValueTimestampWithProducerRecord 
STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldPassIfValueAndTimestampIsEqualForCompareValueTimestampWithProducerRecord 
PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullReversForCompareKeyValueTimestamp STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullReversForCompareKeyValueTimestamp PASSED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullReverseForCompareValueTimestampWithProducerRecord
 STARTED

org.apache.kafka.streams.test.OutputVerifierTest > 
shouldFailIfValueIsDifferentWithNullReverseForCompareValueTimestampWithProducerRecord
 PASSED

org.apache.kafka.streams.MockProcessorContextTest > 
shouldStoreAndReturnStateStores STARTED

org.apache.kafka.streams.MockProcessorContextTest > 
shouldStoreAndReturnStateStores PASSED

Jenkins build is back to normal : Kafka » kafka-trunk-jdk8 #88

[Apache Jenkins Server]

See 

Re: [VOTE] KIP-590: Redirect Zookeeper Mutation Protocols to The Controller

[Colin McCabe]

On Fri, Sep 25, 2020, at 10:49, Boyang Chen wrote:
> Hey Jun,
> 
> On Fri, Sep 25, 2020 at 10:19 AM Jun Rao  wrote:
> 
> > Hi, Boyang,
> >
> > Does EnvelopeRequest avoid the need for IBP? How do we know if the
> > controller supports EnvelopeRequest or not?
> >
> > Unfortunately, the EnvelopeRequest is solving the inter-broker
> > communication problem only. Admin clients still need to learn the proper
> > ApiVersion from the broker, which means we need to bump IBP to limit the
> > version range.
> 

Right-- the purpose of EnvelopeRequest is to avoid downconversion / 
upconversion on the forwarding broker.  It unfortunately doesn't avoid the need 
to tie ApiVersionsResponse to IBP.

> > > On Thu, Sep 24, 2020 at 4:53 PM Jun Rao  wrote:
> > >
> > > > Hi, Jason,
> > > >
> > > > Yes, the most important thing is to be able to avoid two rolling
> > > > restarts
> > > > in the future. If we have a path to achieve that down the road, the
> > > > changes here are fine.
> > > >

Yeah.  I think it would be good to make IBP a feature flag, as long as it could 
be changed without doing a second rolling restart.  We actually don't want to 
have too many feature flags, since it blows up the test matrix.

best,
Colin

> > > > Thanks,
> > > >
> > > > Jun
> > > >
> > > > On Thu, Sep 24, 2020 at 3:20 PM Jason Gustafson 
> > > > wrote:
> > > >
> > > > > > One of the goals of KIP-584 (feature versioning) is that we can get
> > > rid
> > > > > of
> > > > > IBP in the future. So does this change prevent us from removing IBP
> > in
> > > > the
> > > > > future?
> > > > >
> > > > > That is a good question. I think the problem here is that request
> > > > > forwarding puts an expectation on api version support which covers
> > more
> > > > > than one broker. This is why the normal ApiVersions behavior doesn't
> > > > work.
> > > > > I thought about this a bit and haven't come up with a good
> > alternative.
> > > > One
> > > > > thought I've been considering is letting the controller in the
> > > > post-kip-500
> > > > > world set the maximum range of api support for the cluster. However,
> > > even
> > > > > then we would need some way to tell when the controller quorum itself
> > > is
> > > > > ready to enable support for a new api version. My feeling is that we
> > > will
> > > > > probably always need something like the IBP to control when it is
> > safe
> > > to
> > > > > expose versions of APIs which have a cross-broker dependence.
> > However,
> > > > > KIP-584 would still allow us to manage the IBP at the level of a
> > > feature
> > > > so
> > > > > that we don't need two rolling restarts anymore.
> > > > >
> > > > > Best,
> > > > > Jason
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Thu, Sep 24, 2020 at 1:59 PM Jun Rao  wrote:
> > > > >
> > > > > > Hi, Boyang,
> > > > > >
> > > > > > One of the goals of KIP-584 (feature versioning) is that we can get
> > > rid
> > > > > of
> > > > > > IBP in the future. So does this change prevent us from removing IBP
> > > in
> > > > > the
> > > > > > future?
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Jun
> > > > > >
> > > > > > On Thu, Sep 24, 2020 at 12:46 PM Jason Gustafson <
> > ja...@confluent.io
> > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Hey Boyang,
> > > > > > >
> > > > > > > Thanks for the update. This seems like the best thing we can do.
> > > The
> > > > > > > alternative would be to always ensure that the forwarded APIs are
> > > > safe
> > > > > > for
> > > > > > > conversion between versions, but that would restrict the
> > > flexibility
> > > > > that
> > > > > > > the versioning is providing. It would also be a large effort to
> > > avoid
> > > > > > > introducing regressions through conversion. Sadly this broadens
> > the
> > > > > scope
> > > > > > > of the IBP, but in fact forwarded APIs are inter-broker APIs.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jason
> > > > > > >
> > > > > > > On Thu, Sep 24, 2020 at 9:23 AM Boyang Chen <
> > > > > reluctanthero...@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hey there,
> > > > > > > >
> > > > > > > > we spotted a necessary case to handle the redirect request
> > > > > versioning,
> > > > > > > and
> > > > > > > > proposed the following changes:
> > > > > > > >
> > > > > > > > 1. For redirection RPCs (AlterConfig, Acl, Token etc), the
> > > > > > corresponding
> > > > > > > > allowed versions in the ApiVersionResponse will be affected by
> > > the
> > > > > > entire
> > > > > > > > cluster's versioning, not just the receiving broker, since we
> > > need
> > > > to
> > > > > > > > ensure the chosen version get properly handled by all parties.
> > > Thus
> > > > > > from
> > > > > > > > now on, RPC with redirection will be treated as inter-broker
> > RPC,
> > > > and
> > > > > > any
> > > > > > > > version bump for these RPCs has to go through IBP bump as well.
> > > > > > > > ApiVersionResponse will take IBP into considerations for the
> > > > > > redirection
> > > > > > > 

Re: [DISCUSS] KIP-631: The Quorum-based Kafka Controller

[Colin McCabe]

On Fri, Sep 25, 2020, at 10:17, Jun Rao wrote:
> Hi, Colin,
> 
> Thanks for the reply.
> 
> 60. Yes, I think you are right. We probably need the controller id when a
> broker starts up. A broker only stores the Raft leader id in the metadata
> file. To do the initial fetch to the Raft leader, it needs to know the
> host/port associated with the leader id.
> 
> 62. It seems there are 2 parts to this : (1) which listener a client should
> use to initiate a connection to the controller and (2) which listener
> should a controller use to accept client requests. For (1), at any point of
> time, a client only needs to use one listener. I think
> controller.listener.name is meant for the client.

Hi Jun,

controller.listener.names is also used by the controllers.  In the case where 
we have a broker and a controller in the same JVM, we have a single config 
file.  Then we need to know which listeners belong to the controller and which 
belong to the broker component.  That's why it's a list.

> So, a single value seems
> to make more sense. Currently, we don't have a configuration for (2). We
> could add a new one for that and support a list. I am wondering how useful
> it will be. One example that I can think of is that we can reject
> non-controller related requests if accepted on controller-only listeners.
> However, we already support separate authentication for the controller
> listener. So, not sure how useful it is.

The controller always has a separate listener and does not share listeners with 
the broker.  The main reason for this is to avoid giving clients the ability to 
launch a denial-of-service attack on the controller by flooding a broker port.  
A lot of times, these attacks are made unintentionally by poorly coded or 
configured clients.  Additionally, broker ports can also be very busy with 
large fetch requests, and so on.  It's just a bad configuration in general to 
try to overload the same port for the controller and broker, and we don't want 
to allow it.  It would be a regression to go from the current system where 
control requests are safely isolated on a separate port, to one where they're 
not.  It also makes the code and configuration a lot messier.

> 
> 63. (a) I think most users won't know controller.id defaults to broker.id +
> 3000. So, it can be confusing for them to set up controller.connect. If
> this is truly needed, it seems that it's less confusing to make
> controller.id required.
> (b) I am still trying to understand if we truly need to expose a
> controller.id. What if we only expose broker.id and let controller.connect
> just use broker.id? What will be missing?

The controller has a separate ID from the broker, so knowing broker.id is not 
helpful here.

best,
Colin

> 
> Thanks,
> 
> Jun
> 
> On Thu, Sep 24, 2020 at 10:55 PM Colin McCabe  wrote:
> 
> > On Thu, Sep 24, 2020, at 16:24, Jun Rao wrote:
> > > Hi, Colin,
> > >
> > > Thanks for the reply and the updated KIP. A few more comments below.
> > >
> >
> > Hi Jun,
> >
> > >
> > > 53. It seems that you already incorporated the changes in KIP-516. With
> > > topic ids, we don't need to wait for the topic's data to be deleted
> > before
> > > removing the topic metadata. If the topic is recreated, we can still
> > delete
> > > the data properly based on the topic id. So, it seems that we can remove
> > > TopicRecord.Deleting.
> > >
> >
> > Thanks for the reply.  What I was thinking of doing here was using topic
> > IDs internally, but still using names externally.  So the topic UUIDs are
> > only for the purpose of associating topics with partitions -- from the
> > user's point of view, topics are still identified by names.
> >
> > You're right that KIP-516 will simplify things, but I'm not sure when that
> > will land, so I wanted to avoid blocking the initial implementation of this
> > KIP on it.
> >
> > >
> > > 55. It seems to me that the current behavior where we favor the current
> > > broker registration is better. This is because uncontrolled broker
> > shutdown
> > > is rare and its impact is less severe since one just needs to wait for
> > the
> > > session timeout before restarting the broker. If a mis-configured broker
> > > replaces an existing broker, the consequence is more severe since it can
> > > cause the leader to be unavailable, a replica to be out of ISR, and add
> > > more load on the leaders etc.
> > >
> >
> > Hmm, that's a good point.  Let me check this a bit more before I change
> > it, though.
> >
> > >
> > > 60. controller.connect=0...@controller0.example.com:9093,
> > > 1...@controller1.example.com:9093,2...@controller2.example.com : Do we 
> > > need to
> > > include the controller id before @? It seems that the host/port is enough
> > > for establishing the connection. It would also be useful to make it clear
> > > that controller.connect replaces quorum.voters in KIP-595.
> > >
> >
> > I discussed this with Jason earlier, and he felt that the controller IDs
> > were needed in this 

Re: [VOTE] KIP-584: Versioning scheme for features

[Colin McCabe]

On Tue, Sep 22, 2020, at 00:43, Kowshik Prakasam wrote:
> Hi all,
> 
> I wanted to let you know that I have made the following changes to the
> KIP-584 write up. The purpose is to ensure the design is correct for a few
> things which came up during implementation:
>

Hi Kowshik,

Thanks for the updates.

> 
> 1. Per FeatureUpdate error code: The UPDATE_FEATURES controller API is no
> longer transactional. Going forward, we allow for individual FeatureUpdate
> to succeed/fail in the request. As a result, the response schema now
> contains an error code per FeatureUpdate as well as a top-level error code.
> Overall this is a better design because it better represents the nature of
> the API: each FeatureUpdate in the request is independent of the other
> updates, and the controller can process/apply these independently to ZK.
> When an UPDATE_FEATURES request fails, this new design provides better
> clarity to the caller on which FeatureUpdate could not be applied (via the
> individual error codes). In the previous design, we were unable to achieve
> such an increased level of clarity in communicating the error codes.
> 

OK

>
> 2. Due to #1, there were some minor changes required to the proposed Admin
> APIs (describeFeatures and updateFeatures). A few unnecessary public APIs
> have been removed, and couple essential ones have been added. The latest
> changes now represent the latest design.
> 
> 3. The timeoutMs field has been removed from the the UPDATE_FEATURES API
> request, since it was not found to be required during implementation.
> 

Please don't get rid of timeoutMs.  timeoutMs is required if you want to 
implement the ability to timeout the call if the controller can't get to it in 
time.  This is important for avoiding congestion collapse where the controller 
collapses under the weight of lots of retries of the same set of calls.

We may not be able to do it in the initial implementation, but we will 
eventually implement this for all the controller-bound RPCs.

> >
> > 2. Finalized feature version epoch data type has been made to be int32
> > (instead of int64). The reason is that the epoch value is the value of ZK
> > node version, whose data type is int32.
> >

Sorry, I missed this earlier.  Using 16 bit feature levels seems fine.  
However, please don't use a 32-bit epoch here.  We deliberately made the epoch 
64 bits to avoid overflow problems in the future once ZK is gone.

best,
Colin

> > 3. Introduced a new 'status' field in the '/features' ZK node schema. The
> > purpose is to implement Colin's earlier point for the strategy for
> > transitioning from not having a /features znode to having one. An
> > explanation has been provided in the following section of the KIP detailing
> > the different cases:
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP584:Versioningschemeforfeatures-FeatureZKnodestatus
> > .
> >
> > Please let me know if you have any questions or concerns.
> >
> >
> > Cheers,
> > Kowshik
> >
> >
> >
> > Cheers,
> > Kowshik
> >
> > On Tue, Apr 28, 2020 at 11:24 PM Kowshik Prakasam 
> > wrote:
> >
> >> Hi all,
> >>
> >> This KIP vote has been open for ~12 days. The summary of the votes is
> >> that we have 3 binding votes (Colin, Guozhang, Jun), and 3 non-binding
> >> votes (David, Dhruvil, Boyang). Therefore, the KIP vote passes. I'll mark
> >> KIP as accepted and start working on the implementation.
> >>
> >> Thanks a lot!
> >>
> >>
> >> Cheers,
> >> Kowshik
> >>
> >> On Mon, Apr 27, 2020 at 12:15 PM Colin McCabe  wrote:
> >>
> >>> Thanks, Kowshik.  +1 (binding)
> >>>
> >>> best,
> >>> Colin
> >>>
> >>> On Sat, Apr 25, 2020, at 13:20, Kowshik Prakasam wrote:
> >>> > Hi Colin,
> >>> >
> >>> > Thanks for the explanation! I agree with you, and I have updated the
> >>> > KIP.
> >>> > Here is a link to relevant section:
> >>> >
> >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Controller:ZKnodebootstrapwithdefaultvalues
> >>> >
> >>> >
> >>> > Cheers,
> >>> > Kowshik
> >>> >
> >>> > On Fri, Apr 24, 2020 at 8:50 PM Colin McCabe 
> >>> wrote:
> >>> >
> >>> > > On Fri, Apr 24, 2020, at 00:01, Kowshik Prakasam wrote:
> >>> > > > (Kowshik): Great point! However for case #1, I'm not sure why we
> >>> need to
> >>> > > > create a '/features' ZK node with disabled features. Instead, do
> >>> you see
> >>> > > > any drawback if we just do not create it? i.e. if IBP is less than
> >>> 2.6,
> >>> > > the
> >>> > > > controller treats the case as though the versioning system is
> >>> completely
> >>> > > > disabled, and would not create a non-existing '/features' node.
> >>> > >
> >>> > > Hi Kowshik,
> >>> > >
> >>> > > When the IBP is less than 2.6, but the software has been upgraded to
> >>> a
> >>> > > state where it supports this KIP, that
> >>> > >  means the user is upgrading from an earlier version of the
> >>> software.  In
> >>> > > this case, 

Re: [DISCUSS] KIP-630: Kafka Raft Snapshot

[Jose Garcia Sancio]

Thanks for the detailed feedback Jun.

The changes are here:
https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=158864763=25=24

Here is a summary of the change to the KIP:
1. Use end offset for snapshot and snapshot id.
2. Include default for all of the new configuration options.
3. Provide more detail in the response handling for FetchSnapshot

> 20. "Metadata Cache: The component that generates snapshots, reads
> snapshots and reads logs for observer replicas of the topic partition
> __cluster_metadata." It seems this is needed on every broker, not just
> observers?

Yes. I think we need some clarification and consensus here. Some
people are advocating for Kafka brokers to only be observers and would
only contain a Metadata Cache. With the Kafka Controllers being
separate nodes that are voters (follower, candidate or leader) and not
observers. Others are advocating for Kafka Brokers to be able to host
both the Kafka Controller and the Metadata Cache. In this case if the
Controller and Metadata Cache are sharing the same underlying topic
partition then we need to make sure that we unify the snapshotting
logic.

I would like to be able to unify the in-memory state for both the
Kafka Controller and the Metadata Cache so that we can share the same
replicated log and snapshot.

> 21. Our current convention is to use exclusive offset for naming
> checkpoint files. For example, a producer snapshot file of 1234.snapshot
> means that the file includes the producer state up to, but not including
> offset 1234. So, we probably want to follow the same convention for the new
> checkpoint file.

Thanks for pointing this out. This sounds good to me. This was a
detail that I was struggling with when reading the replication code.
Updated the KIP. Wherever the offset is exclusive, I renamed it to
"end offset" (EndOffset).

> 22. Snapshot Format: KIP-631 only defines the format for individual
> records. It seems that we need to define the container format here. For
> example, we need to store the length of each record. Also, does the
> snapshot file need a CRC field?

Yes. I have added more information on this. In summary, we are going
to use Kafka's log format version 2. This will give us support for
compression and CRC at the record batch level. The Kafka Controller
and Metadata Cache can control how big they want the batches to be.

> 23. Could we provide the default value for the new
> configs controller.snapshot.minimum.records and max.replication.lag.ms.
> Also, max.replication.lag.ms seems to just control the snapshot frequency
> by time and not directly relate to replication. So, maybe it should be
> called sth like controller.snapshot.minimum.interval.ms?

"max.replication.lag.ms" is very similar to "replica.lag.time.max.ms".
Kafka uses "replica.lag.time.max.ms" to make progress on the
high-watermark when replicas are slow or offline. We want to use
"max.replication.lag.ms" to make progress on the LBO when replicas are
slow or offline. These very similar names are confusing. How about
"replica.lbo.lag.time.max.ms"?

How often snapshotting will happen is determined by
"controller.snapshot.minimum.records".

> 24. "Kafka allows the clients to delete records that are less than a given
> offset by using the DeleteRecords RPC . Those requests will be validated
> using the same logic enumerated above." Hmm, should we allow deleteRecord
> on the metadata topic? If we do, does it trim the snapshot accordingly?

Yeah. After thinking about it some more, I don't think we shouldn't
allow DeleteRecords to succeed on the __cluster_metadata partition.
For the error that we return it looks like our options are the
existing "POLICY_VIOLATIOIN" (the description for this error is
"Request parameters do not satisfy the configured policy.') or
introduce a new error. I think we should just return
POLICY_VIOLATIOIN, what do you think?

> 25. "The followers of the __cluster_metadata topic partition will
> concurrently fetch the snapshot and replicated log. This means that
> candidates with incomplete snapshots will send a vote request with a
> LastOffsetEpoch of -1 and a LastOffset of -1 no matter the LEO of the
> replicated log." My understanding is that a follower will either fetch from
> the snapshot or the log, but not both at the same time. Could you explain
> how the concurrent part works? Also, what's an incomplete snapshot?

Yes. I rewrote this section based on your comment and Jason's
comments. Let me know if this addresses your concerns.

https://cwiki.apache.org/confluence/display/KAFKA/KIP-630:+Kafka+Raft+Snapshot#KIP630:KafkaRaftSnapshot-ChangestoLeaderElection

>
> 26. FetchRequest:
> 26.1 Handling Fetch Request: I agree with Jason that SnapshotOffsetAndEpoch
> already tells us the next offset to fetch. So, we don't need to
> set NextOffsetAndEpoch in the response.

Agreed. The response will set one or the other. If SnapshotId (field
renamed in the latest version of the KIP) is set then the 

Build failed in Jenkins: Kafka » kafka-trunk-jdk11 #89

[Apache Jenkins Server]

See 


Changes:

[github] KAFKA-10519; Add missing unit test for `VotedState` (#9337)

[github] MINOR: Use the automated protocol for the Consumer Protocol's 
subscriptions and assignments (#8897)


--
[...truncated 3.35 MB...]
org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTime[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTime[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldSetRecordMetadata[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldSetRecordMetadata[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForLargerValue[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForLargerValue[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessRecordForTopic[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessRecordForTopic[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldForwardRecordsFromSubtopologyToSubtopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldForwardRecordsFromSubtopologyToSubtopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForSmallerValue[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForSmallerValue[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCreateStateDirectoryForStatefulTopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCreateStateDirectoryForStatefulTopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateIfWallClockTimeAdvances[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateIfWallClockTimeAdvances[Eos enabled = false] PASSED

org.apache.kafka.streams.test.TestRecordTest > testConsumerRecord STARTED

org.apache.kafka.streams.test.TestRecordTest > testConsumerRecord PASSED

org.apache.kafka.streams.test.TestRecordTest > testToString STARTED

org.apache.kafka.streams.test.TestRecordTest > testToString PASSED

org.apache.kafka.streams.test.TestRecordTest > testInvalidRecords STARTED

org.apache.kafka.streams.test.TestRecordTest > testInvalidRecords PASSED

org.apache.kafka.streams.test.TestRecordTest > testPartialConstructorEquals 
STARTED

org.apache.kafka.streams.test.TestRecordTest > testPartialConstructorEquals 
PASSED

org.apache.kafka.streams.test.TestRecordTest > testMultiFieldMatcher STARTED

org.apache.kafka.streams.test.TestRecordTest > testMultiFieldMatcher PASSED

org.apache.kafka.streams.test.TestRecordTest > testFields STARTED

org.apache.kafka.streams.test.TestRecordTest > testFields PASSED

org.apache.kafka.streams.test.TestRecordTest > testProducerRecord STARTED

org.apache.kafka.streams.test.TestRecordTest > testProducerRecord PASSED

org.apache.kafka.streams.test.TestRecordTest > testEqualsAndHashCode STARTED

org.apache.kafka.streams.test.TestRecordTest > testEqualsAndHashCode PASSED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldWorkWithInMemoryStore STARTED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldWorkWithInMemoryStore PASSED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldFailWithLogging STARTED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldFailWithLogging PASSED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldFailWithCaching STARTED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 

Re: [VOTE] KIP-584: Versioning scheme for features

[Jun Rao]

Hi, Kowshik,

Thanks for the update. Those changes seem fine to me.

One of the goals listed in this KIP is the removal of IBP in the future.
Recent discussion in KIP-590 intends to expand the scope of IBP for
forwarding requests to the controller. In light of this, is the goal of
removing IBP still valid?

Thanks,

Jun

On Tue, Sep 22, 2020 at 12:43 AM Kowshik Prakasam 
wrote:

> Hi all,
>
> I wanted to let you know that I have made the following changes to the
> KIP-584 write up. The purpose is to ensure the design is correct for a few
> things which came up during implementation:
>
> 1. Per FeatureUpdate error code: The UPDATE_FEATURES controller API is no
> longer transactional. Going forward, we allow for individual FeatureUpdate
> to succeed/fail in the request. As a result, the response schema now
> contains an error code per FeatureUpdate as well as a top-level error code.
> Overall this is a better design because it better represents the nature of
> the API: each FeatureUpdate in the request is independent of the other
> updates, and the controller can process/apply these independently to ZK.
> When an UPDATE_FEATURES request fails, this new design provides better
> clarity to the caller on which FeatureUpdate could not be applied (via the
> individual error codes). In the previous design, we were unable to achieve
> such an increased level of clarity in communicating the error codes.
>
> 2. Due to #1, there were some minor changes required to the proposed Admin
> APIs (describeFeatures and updateFeatures). A few unnecessary public APIs
> have been removed, and couple essential ones have been added. The latest
> changes now represent the latest design.
>
> 3. The timeoutMs field has been removed from the the UPDATE_FEATURES API
> request, since it was not found to be required during implementation.
>
> 4. Previously we handled the incompatible broker lifetime race condition in
> the controller by skipping sending of UpdateMetadataRequest to the
> incompatible broker. But this had a few edge cases. Instead, now we handle
> it by refusing to register the incompatible broker in the controller. This
> is a better design because if we already acted on an incompatible broker
> registration, then some damage may already be done to the cluster. This is
> because the UpdatateMetadataRequest will still be sent to other brokers and
> its metadata will be available to the clients. Therefore we would like to
> avoid this problem with the new design where the controller would not keep
> track of an incompatible broker because the broker will eventually shutdown
> automatically (when reacting to the incompatibility).
>
> Please let me know if you have any questions.
>
>
> Cheers,
> Kowshik
>
>
> On Mon, Jun 8, 2020 at 3:32 AM Kowshik Prakasam 
> wrote:
>
> > Hi all,
> >
> > I wanted to let you know that I have made the following minor changes to
> > the KIP-584  write up.
> The
> > purpose is to ensure the design is correct for a few things which came up
> > during implementation:
> >
> > 1. Feature version data type has been made to be int16 (instead of
> int64).
> > The reason is two fold:
> > a. Usage of int64 felt overkill. Feature version bumps are infrequent
> > (since these bumps represent breaking changes that are generally
> > infrequent). Therefore int16 is big enough to support version bumps of a
> > particular feature.
> > b. The int16 data type aligns well with existing API versions data
> > type. Please see the file
> >
> '/clients/src/main/resources/common/message/ApiVersionsResponse.json'.
> >
> > 2. Finalized feature version epoch data type has been made to be int32
> > (instead of int64). The reason is that the epoch value is the value of ZK
> > node version, whose data type is int32.
> >
> > 3. Introduced a new 'status' field in the '/features' ZK node schema. The
> > purpose is to implement Colin's earlier point for the strategy for
> > transitioning from not having a /features znode to having one. An
> > explanation has been provided in the following section of the KIP
> detailing
> > the different cases:
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP584:Versioningschemeforfeatures-FeatureZKnodestatus
> > .
> >
> > Please let me know if you have any questions or concerns.
> >
> >
> > Cheers,
> > Kowshik
> >
> >
> >
> > Cheers,
> > Kowshik
> >
> > On Tue, Apr 28, 2020 at 11:24 PM Kowshik Prakasam <
> kpraka...@confluent.io>
> > wrote:
> >
> >> Hi all,
> >>
> >> This KIP vote has been open for ~12 days. The summary of the votes is
> >> that we have 3 binding votes (Colin, Guozhang, Jun), and 3 non-binding
> >> votes (David, Dhruvil, Boyang). Therefore, the KIP vote passes. I'll
> mark
> >> KIP as accepted and start working on the implementation.
> >>
> >> Thanks a lot!
> >>
> >>
> >> Cheers,
> >> Kowshik
> >>
> >> On Mon, Apr 27, 2020 at 12:15 PM Colin McCabe 
> wrote:
> >>
> >>> 

Re: [DISCUSS] KIP-673: Emit JSONs with new auto-generated schema

[Anastasia Vela]

Hi Tom,

Thanks for your input!

1. I'll add more details for the RequestConvertToJson and
XYZJsonDataConverter classes. Hopefully it will be more clear, but just to
answer your question, RequestConvertToJson does not return a
XYZJsonDataConverter, but rather it returns a JsonNode which will be
serialized. The JsonDataConverter is the new auto-generated schema for each
request/response type that contains the method to return the JsonNode to be
serialized.

2. There is no defined order of the properties, rather it's in the order
that it is set in. So if you first set key B, then key A, the properties
would appear with key B first. JsonNodes when serialized does not sort the
keys.

3. Yes, serialization is done via Jackson databind.

Thanks again,
Anastasia

On Fri, Sep 25, 2020 at 1:15 AM Tom Bentley  wrote:

> Hi Anastasia,
>
> Thanks for the KIP, I can certainly see the benefit of this. I have a few
> questions:
>
> 1. I think it would be helpful to readers to explicitly illustrate the
> RequestConvertToJson and XYZJsonDataConverter classes (e.g. with method
> signatures for one or two methods), because currently it's not clear (to me
> at least) exactly what's being proposed. Does the RequestConvertToJson
> return a XYZJsonDataConverter?
>
> 2. Does the serialization have a defined order of properties (alphabetic
> perhaps)? My concern here is that properties appearing in order according
> to how they are iterated in a hash map might harm human readability of the
> logs.
>
> 3. Would the serialization be done via the Jackson databinding?
>
> Many thanks,
>
> Tom
>
> On Thu, Sep 24, 2020 at 11:49 PM Anastasia Vela 
> wrote:
>
> > Hi all,
> >
> > I'd like to discuss KIP-673:
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-673%3A+Emit+JSONs+with+new+auto-generated+schema
> >
> > This is a proposal to change the format of request and response traces to
> > JSON, which would be easier to load and parse, because the current format
> > is only JSON-like and not easily parsable.
> >
> > Let me know what you think,
> > Anastasia
> >
>

Re: [VOTE] KIP-590: Redirect Zookeeper Mutation Protocols to The Controller

[Boyang Chen]

Hey Jun,

On Fri, Sep 25, 2020 at 10:19 AM Jun Rao  wrote:

> Hi, Boyang,
>
> Does EnvelopeRequest avoid the need for IBP? How do we know if the
> controller supports EnvelopeRequest or not?
>
> Unfortunately, the EnvelopeRequest is solving the inter-broker
communication problem only. Admin clients still need to learn the proper
ApiVersion from the broker, which means we need to bump IBP to limit the
version range.

Boyang


> Thanks,
>
> Jun
>
> On Thu, Sep 24, 2020 at 6:22 PM Boyang Chen 
> wrote:
>
> > Hey Jason and Jun,
> >
> > thanks for the reply. Actually after some offline discussion, we have
> seen
> > hassles around upgrading and downgrading RPCs during redirection, which
> is
> > an error-prone approach to coordinate all parties to choose the correct
> > version to handle. Alternatively, we propose to bring back the
> > EnvelopeRequest to solve this problem, by embedding the admin request
> data
> > inside the request with consistent version. The complete workflow looks
> > like:
> >
> > 1. broker authorizes all accesses and strips out rejected stuff
> > 2. broker forwards envelope of authorized actions in envelope
> > 3. controller checks cluster_action for envelope request
> > 4. if check passes, then all actions in the request are assumed to be
> > authorized
> >
> > Also we need to point out that we are not talking about going backwards.
> > This workflow restricts the Envelope RPC with cluster_action permission
> to
> > reduce the risk of impersonation at best effort. Additionally, we are not
> > proposing any incompatible changes such as principal serialization. We
> > shall still use the split and join semantic we built as of current to
> only
> > forward authenticated resources.
> >
> > Let me know if this makes sense.
> >
> > Boyang
> >
> > On Thu, Sep 24, 2020 at 4:53 PM Jun Rao  wrote:
> >
> > > Hi, Jason,
> > >
> > > Yes, the most important thing is to be able to avoid two rolling
> restarts
> > > in the future. If we have a path to achieve that down the road, the
> > changes
> > > here are fine.
> > >
> > > Thanks,
> > >
> > > Jun
> > >
> > > On Thu, Sep 24, 2020 at 3:20 PM Jason Gustafson 
> > > wrote:
> > >
> > > > > One of the goals of KIP-584 (feature versioning) is that we can get
> > rid
> > > > of
> > > > IBP in the future. So does this change prevent us from removing IBP
> in
> > > the
> > > > future?
> > > >
> > > > That is a good question. I think the problem here is that request
> > > > forwarding puts an expectation on api version support which covers
> more
> > > > than one broker. This is why the normal ApiVersions behavior doesn't
> > > work.
> > > > I thought about this a bit and haven't come up with a good
> alternative.
> > > One
> > > > thought I've been considering is letting the controller in the
> > > post-kip-500
> > > > world set the maximum range of api support for the cluster. However,
> > even
> > > > then we would need some way to tell when the controller quorum itself
> > is
> > > > ready to enable support for a new api version. My feeling is that we
> > will
> > > > probably always need something like the IBP to control when it is
> safe
> > to
> > > > expose versions of APIs which have a cross-broker dependence.
> However,
> > > > KIP-584 would still allow us to manage the IBP at the level of a
> > feature
> > > so
> > > > that we don't need two rolling restarts anymore.
> > > >
> > > > Best,
> > > > Jason
> > > >
> > > >
> > > >
> > > >
> > > > On Thu, Sep 24, 2020 at 1:59 PM Jun Rao  wrote:
> > > >
> > > > > Hi, Boyang,
> > > > >
> > > > > One of the goals of KIP-584 (feature versioning) is that we can get
> > rid
> > > > of
> > > > > IBP in the future. So does this change prevent us from removing IBP
> > in
> > > > the
> > > > > future?
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jun
> > > > >
> > > > > On Thu, Sep 24, 2020 at 12:46 PM Jason Gustafson <
> ja...@confluent.io
> > >
> > > > > wrote:
> > > > >
> > > > > > Hey Boyang,
> > > > > >
> > > > > > Thanks for the update. This seems like the best thing we can do.
> > The
> > > > > > alternative would be to always ensure that the forwarded APIs are
> > > safe
> > > > > for
> > > > > > conversion between versions, but that would restrict the
> > flexibility
> > > > that
> > > > > > the versioning is providing. It would also be a large effort to
> > avoid
> > > > > > introducing regressions through conversion. Sadly this broadens
> the
> > > > scope
> > > > > > of the IBP, but in fact forwarded APIs are inter-broker APIs.
> > > > > >
> > > > > > Thanks,
> > > > > > Jason
> > > > > >
> > > > > > On Thu, Sep 24, 2020 at 9:23 AM Boyang Chen <
> > > > reluctanthero...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hey there,
> > > > > > >
> > > > > > > we spotted a necessary case to handle the redirect request
> > > > versioning,
> > > > > > and
> > > > > > > proposed the following changes:
> > > > > > >
> > > > > > > 1. For redirection RPCs (AlterConfig, Acl, Token etc), the
> > > > > 

Build failed in Jenkins: Kafka » kafka-trunk-jdk8 #87

[Apache Jenkins Server]

See 


Changes:

[github] KAFKA-10516; Disable automatic retry of `THROTTLING_QUOTA_EXCEEDED` 
errors in the `kafka-topics` command (KIP-599) (#9334)


--
[...truncated 3.32 MB...]

org.apache.kafka.streams.TopologyTestDriverTest > shouldRespectTaskIdling[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUseSourceSpecificDeserializers[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUseSourceSpecificDeserializers[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldReturnAllStores[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldReturnAllStores[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotCreateStateDirectoryForStatelessTopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotCreateStateDirectoryForStatelessTopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldApplyGlobalUpdatesCorrectlyInRecursiveTopologies[Eos enabled = false] 
STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldApplyGlobalUpdatesCorrectlyInRecursiveTopologies[Eos enabled = false] 
PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnAllStoresNames[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnAllStoresNames[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPassRecordHeadersIntoSerializersAndDeserializers[Eos enabled = false] 
STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPassRecordHeadersIntoSerializersAndDeserializers[Eos enabled = false] 
PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessConsumerRecordList[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessConsumerRecordList[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUseSinkSpecificSerializers[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUseSinkSpecificSerializers[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldFlushStoreForFirstInput[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldFlushStoreForFirstInput[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessFromSourceThatMatchPattern[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessFromSourceThatMatchPattern[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureSinkTopicNamesIfWrittenInto[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureSinkTopicNamesIfWrittenInto[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUpdateStoreForNewKey[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldUpdateStoreForNewKey[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldSendRecordViaCorrectSourceTopicDeprecated[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldSendRecordViaCorrectSourceTopicDeprecated[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTime[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTime[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldSetRecordMetadata[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldSetRecordMetadata[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForLargerValue[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForLargerValue[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] 

Build failed in Jenkins: Kafka » kafka-trunk-jdk11 #88

[Apache Jenkins Server]

See 


Changes:

[github] KAFKA-10516; Disable automatic retry of `THROTTLING_QUOTA_EXCEEDED` 
errors in the `kafka-topics` command (KIP-599) (#9334)


--
[...truncated 3.35 MB...]

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldReturnCorrectInMemoryStoreTypeOnly[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > shouldThrowForMissingTime[Eos 
enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCaptureInternalTopicNamesIfWrittenInto[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateOnWallClockTimeDeprecated[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessRecordForTopic[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldProcessRecordForTopic[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldForwardRecordsFromSubtopologyToSubtopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldForwardRecordsFromSubtopologyToSubtopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForSmallerValue[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldNotUpdateStoreForSmallerValue[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCreateStateDirectoryForStatefulTopology[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldCreateStateDirectoryForStatefulTopology[Eos enabled = false] PASSED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateIfWallClockTimeAdvances[Eos enabled = false] STARTED

org.apache.kafka.streams.TopologyTestDriverTest > 
shouldPunctuateIfWallClockTimeAdvances[Eos enabled = false] PASSED

org.apache.kafka.streams.test.TestRecordTest > testConsumerRecord STARTED

org.apache.kafka.streams.test.TestRecordTest > testConsumerRecord PASSED

org.apache.kafka.streams.test.TestRecordTest > testToString STARTED

org.apache.kafka.streams.test.TestRecordTest > testToString PASSED

org.apache.kafka.streams.test.TestRecordTest > testInvalidRecords STARTED

org.apache.kafka.streams.test.TestRecordTest > testInvalidRecords PASSED

org.apache.kafka.streams.test.TestRecordTest > testPartialConstructorEquals 
STARTED

org.apache.kafka.streams.test.TestRecordTest > testPartialConstructorEquals 
PASSED

org.apache.kafka.streams.test.TestRecordTest > testMultiFieldMatcher STARTED

org.apache.kafka.streams.test.TestRecordTest > testMultiFieldMatcher PASSED

org.apache.kafka.streams.test.TestRecordTest > testFields STARTED

org.apache.kafka.streams.test.TestRecordTest > testFields PASSED

org.apache.kafka.streams.test.TestRecordTest > testProducerRecord STARTED

org.apache.kafka.streams.test.TestRecordTest > testProducerRecord PASSED

org.apache.kafka.streams.test.TestRecordTest > testEqualsAndHashCode STARTED

org.apache.kafka.streams.test.TestRecordTest > testEqualsAndHashCode PASSED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldWorkWithInMemoryStore STARTED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldWorkWithInMemoryStore PASSED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldFailWithLogging STARTED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldFailWithLogging PASSED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldFailWithCaching STARTED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldFailWithCaching PASSED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldWorkWithPersistentStore STARTED

org.apache.kafka.streams.test.wordcount.WindowedWordCountProcessorTest > 
shouldWorkWithPersistentStore PASSED

org.apache.kafka.streams.internals.KeyValueStoreFacadeTest > shouldReturnIsOpen 
STARTED

org.apache.kafka.streams.internals.KeyValueStoreFacadeTest > shouldReturnIsOpen 
PASSED

org.apache.kafka.streams.internals.KeyValueStoreFacadeTest > 
shouldDeleteAndReturnPlainValue STARTED

org.apache.kafka.streams.internals.KeyValueStoreFacadeTest > 
shouldDeleteAndReturnPlainValue PASSED

org.apache.kafka.streams.internals.KeyValueStoreFacadeTest > shouldReturnName 
STARTED

org.apache.kafka.streams.internals.KeyValueStoreFacadeTest > shouldReturnName 
PASSED

Re: [VOTE] KIP-590: Redirect Zookeeper Mutation Protocols to The Controller

[Jun Rao]

Hi, Boyang,

Does EnvelopeRequest avoid the need for IBP? How do we know if the
controller supports EnvelopeRequest or not?

Thanks,

Jun

On Thu, Sep 24, 2020 at 6:22 PM Boyang Chen 
wrote:

> Hey Jason and Jun,
>
> thanks for the reply. Actually after some offline discussion, we have seen
> hassles around upgrading and downgrading RPCs during redirection, which is
> an error-prone approach to coordinate all parties to choose the correct
> version to handle. Alternatively, we propose to bring back the
> EnvelopeRequest to solve this problem, by embedding the admin request data
> inside the request with consistent version. The complete workflow looks
> like:
>
> 1. broker authorizes all accesses and strips out rejected stuff
> 2. broker forwards envelope of authorized actions in envelope
> 3. controller checks cluster_action for envelope request
> 4. if check passes, then all actions in the request are assumed to be
> authorized
>
> Also we need to point out that we are not talking about going backwards.
> This workflow restricts the Envelope RPC with cluster_action permission to
> reduce the risk of impersonation at best effort. Additionally, we are not
> proposing any incompatible changes such as principal serialization. We
> shall still use the split and join semantic we built as of current to only
> forward authenticated resources.
>
> Let me know if this makes sense.
>
> Boyang
>
> On Thu, Sep 24, 2020 at 4:53 PM Jun Rao  wrote:
>
> > Hi, Jason,
> >
> > Yes, the most important thing is to be able to avoid two rolling restarts
> > in the future. If we have a path to achieve that down the road, the
> changes
> > here are fine.
> >
> > Thanks,
> >
> > Jun
> >
> > On Thu, Sep 24, 2020 at 3:20 PM Jason Gustafson 
> > wrote:
> >
> > > > One of the goals of KIP-584 (feature versioning) is that we can get
> rid
> > > of
> > > IBP in the future. So does this change prevent us from removing IBP in
> > the
> > > future?
> > >
> > > That is a good question. I think the problem here is that request
> > > forwarding puts an expectation on api version support which covers more
> > > than one broker. This is why the normal ApiVersions behavior doesn't
> > work.
> > > I thought about this a bit and haven't come up with a good alternative.
> > One
> > > thought I've been considering is letting the controller in the
> > post-kip-500
> > > world set the maximum range of api support for the cluster. However,
> even
> > > then we would need some way to tell when the controller quorum itself
> is
> > > ready to enable support for a new api version. My feeling is that we
> will
> > > probably always need something like the IBP to control when it is safe
> to
> > > expose versions of APIs which have a cross-broker dependence. However,
> > > KIP-584 would still allow us to manage the IBP at the level of a
> feature
> > so
> > > that we don't need two rolling restarts anymore.
> > >
> > > Best,
> > > Jason
> > >
> > >
> > >
> > >
> > > On Thu, Sep 24, 2020 at 1:59 PM Jun Rao  wrote:
> > >
> > > > Hi, Boyang,
> > > >
> > > > One of the goals of KIP-584 (feature versioning) is that we can get
> rid
> > > of
> > > > IBP in the future. So does this change prevent us from removing IBP
> in
> > > the
> > > > future?
> > > >
> > > > Thanks,
> > > >
> > > > Jun
> > > >
> > > > On Thu, Sep 24, 2020 at 12:46 PM Jason Gustafson  >
> > > > wrote:
> > > >
> > > > > Hey Boyang,
> > > > >
> > > > > Thanks for the update. This seems like the best thing we can do.
> The
> > > > > alternative would be to always ensure that the forwarded APIs are
> > safe
> > > > for
> > > > > conversion between versions, but that would restrict the
> flexibility
> > > that
> > > > > the versioning is providing. It would also be a large effort to
> avoid
> > > > > introducing regressions through conversion. Sadly this broadens the
> > > scope
> > > > > of the IBP, but in fact forwarded APIs are inter-broker APIs.
> > > > >
> > > > > Thanks,
> > > > > Jason
> > > > >
> > > > > On Thu, Sep 24, 2020 at 9:23 AM Boyang Chen <
> > > reluctanthero...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Hey there,
> > > > > >
> > > > > > we spotted a necessary case to handle the redirect request
> > > versioning,
> > > > > and
> > > > > > proposed the following changes:
> > > > > >
> > > > > > 1. For redirection RPCs (AlterConfig, Acl, Token etc), the
> > > > corresponding
> > > > > > allowed versions in the ApiVersionResponse will be affected by
> the
> > > > entire
> > > > > > cluster's versioning, not just the receiving broker, since we
> need
> > to
> > > > > > ensure the chosen version get properly handled by all parties.
> Thus
> > > > from
> > > > > > now on, RPC with redirection will be treated as inter-broker RPC,
> > and
> > > > any
> > > > > > version bump for these RPCs has to go through IBP bump as well.
> > > > > > ApiVersionResponse will take IBP into considerations for the
> > > > redirection
> > > > > > RPCs allowable versions.
> > > > > >
> > > > > > 2. 

Re: [DISCUSS] KIP-631: The Quorum-based Kafka Controller

[Jun Rao]

Hi, Colin,

Thanks for the reply.

60. Yes, I think you are right. We probably need the controller id when a
broker starts up. A broker only stores the Raft leader id in the metadata
file. To do the initial fetch to the Raft leader, it needs to know the
host/port associated with the leader id.

62. It seems there are 2 parts to this : (1) which listener a client should
use to initiate a connection to the controller and (2) which listener
should a controller use to accept client requests. For (1), at any point of
time, a client only needs to use one listener. I think
controller.listener.name is meant for the client. So, a single value seems
to make more sense. Currently, we don't have a configuration for (2). We
could add a new one for that and support a list. I am wondering how useful
it will be. One example that I can think of is that we can reject
non-controller related requests if accepted on controller-only listeners.
However, we already support separate authentication for the controller
listener. So, not sure how useful it is.

63. (a) I think most users won't know controller.id defaults to broker.id +
3000. So, it can be confusing for them to set up controller.connect. If
this is truly needed, it seems that it's less confusing to make
controller.id required.
(b) I am still trying to understand if we truly need to expose a
controller.id. What if we only expose broker.id and let controller.connect
just use broker.id? What will be missing?

Thanks,

Jun

On Thu, Sep 24, 2020 at 10:55 PM Colin McCabe  wrote:

> On Thu, Sep 24, 2020, at 16:24, Jun Rao wrote:
> > Hi, Colin,
> >
> > Thanks for the reply and the updated KIP. A few more comments below.
> >
>
> Hi Jun,
>
> >
> > 53. It seems that you already incorporated the changes in KIP-516. With
> > topic ids, we don't need to wait for the topic's data to be deleted
> before
> > removing the topic metadata. If the topic is recreated, we can still
> delete
> > the data properly based on the topic id. So, it seems that we can remove
> > TopicRecord.Deleting.
> >
>
> Thanks for the reply.  What I was thinking of doing here was using topic
> IDs internally, but still using names externally.  So the topic UUIDs are
> only for the purpose of associating topics with partitions -- from the
> user's point of view, topics are still identified by names.
>
> You're right that KIP-516 will simplify things, but I'm not sure when that
> will land, so I wanted to avoid blocking the initial implementation of this
> KIP on it.
>
> >
> > 55. It seems to me that the current behavior where we favor the current
> > broker registration is better. This is because uncontrolled broker
> shutdown
> > is rare and its impact is less severe since one just needs to wait for
> the
> > session timeout before restarting the broker. If a mis-configured broker
> > replaces an existing broker, the consequence is more severe since it can
> > cause the leader to be unavailable, a replica to be out of ISR, and add
> > more load on the leaders etc.
> >
>
> Hmm, that's a good point.  Let me check this a bit more before I change
> it, though.
>
> >
> > 60. controller.connect=0...@controller0.example.com:9093,
> > 1...@controller1.example.com:9093,2...@controller2.example.com : Do we need 
> > to
> > include the controller id before @? It seems that the host/port is enough
> > for establishing the connection. It would also be useful to make it clear
> > that controller.connect replaces quorum.voters in KIP-595.
> >
>
> I discussed this with Jason earlier, and he felt that the controller IDs
> were needed in this configuration key.  It is certainly needed when
> configuring the controllers themselves, since they need to know each
> others' IDs.
>
> >
> > 61. I am not sure that we need both controller.listeners and
> > controller.connect.security.protocol since the former implies the
> security
> > protocol. The reason that we have both inter.broker.listener.name and
> > inter.broker.security.protocol is because we had the latter first and
> later
> > realized that the former is more general.
> >
>
> That's a good point.  I've removed this from the KIP.
>
> >
> > 62. I am still not sure that you need controller.listeners to be a list.
> > All listeners are already defined in listeners. To migrate from plaintext
> > to SSL, one can configure listeners to have both plaintext and SSL. After
> > that, one can just change controller.listeners from plaintext to SSL.
> This
> > is similar to how to change the listener for inter broker connections.
> > Also, controller.listener.name may be a more accurate name?
> >
>
> The issue that I see here is that if you are running with the controller
> and broker in the same JVM, if you define a few listeners in "listeners"
> they will get used as regular broker listeners, unless you put them in
> controller.listeners.  Therefore, controller.listeners needs to be a list.
>
> controller.listener.names does sound like a better name, though... I've
> updated it to that.

[jira] [Resolved] (KAFKA-10519) Unit tests for VotedState

[Jason Gustafson (Jira)]

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

Jason Gustafson resolved KAFKA-10519.
-
Resolution: Fixed

> Unit tests for VotedState
> -
>
> Key: KAFKA-10519
> URL: https://issues.apache.org/jira/browse/KAFKA-10519
> Project: Kafka
>  Issue Type: Sub-task
>Reporter: Jason Gustafson
>Assignee: Jason Gustafson
>Priority: Major
>
> We accidentally checked in an empty test class `VotedStateTest`. We should 
> add missing unit tests.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Created] (KAFKA-10528) Make output of AK various tools consistent in case of errors

[David Jacot (Jira)]

David Jacot created KAFKA-10528:
---

 Summary: Make output of AK various tools consistent in case of 
errors
 Key: KAFKA-10528
 URL: https://issues.apache.org/jira/browse/KAFKA-10528
 Project: Kafka
  Issue Type: Improvement
Reporter: David Jacot


AK has various command line tools. I have noticed that they are not all 
consistent when it comes to how they treat and print out errors. Typically, the 
message of the exception is printed out followed by its stacktrace.

I have noticed couple of things:
* Sometimes the stacktrace is printed out using the logger in some tools or 
with println in others. Also, I wonder if providing the stacktrace to the users 
is a good idea at all as it gives the feeling that the tool did not work even 
though the error may be legitimate.
* Some tools unwrap ExecutionException to provide the message of the real cause 
to the user. Others don't.
* The handling of errors resulting from bad or wrong flags is also inconsistent 
in certain tools. We should review this and be consistent.

There may be other things to look after. I think that we should review all the 
tools and make them consistent.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Re: [DISCUSS] KIP-516: Topic Identifiers

[Lucas Bradstreet]

Hi Ismael,

If you do not store it in a metadata file or in the directory structure
would you then
require the LeaderAndIsrRequest following startup to give you some notion
of
topic name in memory? We would still need this information for the older
protocols, but
perhaps this is what's meant by tech debt.

Once we're free of the old non-topicID requests then I think you wouldn't
need to retain the topic name.
I think the ability to easily look up topic names associated with partition
directories would still be missed
when diagnosing issues, though maybe it wouldn't be a deal breaker with the
right tooling.

Thanks,

Lucas

On Fri, Sep 25, 2020 at 7:55 AM Ismael Juma  wrote:

> Hi Lucas,
>
> Why would you include the name and id? I think you'd want to remove the
> name from the directory name right? Jason's suggestion was that if you
> remove the name from the directory, then why would you need the id name
> mapping file?
>
> Ismael
>
> On Thu, Sep 24, 2020 at 4:24 PM Lucas Bradstreet 
> wrote:
>
> > > 2. Part of the usage of the file is to have persistent storage of the
> > topic
> > ID and use it to compare with the ID supplied in the LeaderAndIsr
> Request.
> > There is some discussion in the KIP about changes to the directory
> > structure, but I believe directory changes were considered to be out of
> > scope when the KIP was written.
> >
> >
> > Yeah, I was hoping to get a better understanding of why it was taken out
> of
> > scope. Perhaps Lucas Bradstreet might have more insight about the
> decision.
> > Basically my point is that we have to create additional infrastructure
> here
> > to support the name/id mapping, so I wanted to understand if we just
> > consider this a sort of tech debt. It would be useful to cover how we
> > handle the case when this file gets corrupted. Seems like we just have to
> > trust that it matches whatever the controller tells us and rewrite it?
> >
> >
> > Hi Jason, Justine,
> >
> > My thought process is that we will likely need the metadata file whether
> we
> > rename the directories or not.
> > Linux supports filenames of up to 255 bytes and that would not be enough
> to
> > support a directory name
> >  including both the name and topic ID. Given that fact, it seemed
> > reasonable to add the metadata file
> > and leave the directory rename until some time in the future (possibly
> > never).
> >
> > If the file does get corrupted I think you're right. We would either have
> > to trust it matches what the controller tells us
> >  or error out and let an administrator resolve it by checking across
> > replicas for consistency.
> >
> > Lucas
> >
> >
> > On Thu, Sep 24, 2020 at 3:41 PM Jason Gustafson 
> > wrote:
> >
> > > Thanks Justine. Responses below:
> > >
> > > > 1. Yes, the directory will still be based on the topic names.
> > > LeaderAndIsrRequest is one of the few requests that will still contain
> > the
> > > topic name. So I think we have this covered. Sorry for confusion.
> > >
> > > Ah, you're right. My eyes passed right over the field.
> > >
> > > > 2. Part of the usage of the file is to have persistent storage of the
> > > topic
> > > ID and use it to compare with the ID supplied in the LeaderAndIsr
> > Request.
> > > There is some discussion in the KIP about changes to the directory
> > > structure, but I believe directory changes were considered to be out of
> > > scope when the KIP was written.
> > >
> > > Yeah, I was hoping to get a better understanding of why it was taken
> out
> > of
> > > scope. Perhaps Lucas Bradstreet might have more insight about the
> > decision.
> > > Basically my point is that we have to create additional infrastructure
> > here
> > > to support the name/id mapping, so I wanted to understand if we just
> > > consider this a sort of tech debt. It would be useful to cover how we
> > > handle the case when this file gets corrupted. Seems like we just have
> to
> > > trust that it matches whatever the controller tells us and rewrite it?
> > >
> > > > 3. I think this is a good point, but I again I wonder about the scope
> > of
> > > the KIP. Most of the changes mentioned in the KIP are for supporting
> > topic
> > > deletion and I believe that is why the produce request was listed under
> > > future work.
> > >
> > > That's fair. I brought it up since `Fetch` is already included. If
> we've
> > > got `Metadata` and `Fetch`, seems we may as well do `Produce` and save
> an
> > > extra kip. No strong objection though if you want to leave it out.
> > >
> > >
> > > -Jason
> > >
> > >
> > > On Thu, Sep 24, 2020 at 3:26 PM Justine Olshan 
> > > wrote:
> > >
> > > > Hi Jason,
> > > >
> > > > Thanks for your comments.
> > > >
> > > > 1. Yes, the directory will still be based on the topic names.
> > > > LeaderAndIsrRequest is one of the few requests that will still
> contain
> > > the
> > > > topic name. So I think we have this covered. Sorry for confusion.
> > > >
> > > > 2. Part of the usage of the file is to have persistent storage of 

[jira] [Resolved] (KAFKA-10516) Implement Topic Command changes

[David Jacot (Jira)]

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

David Jacot resolved KAFKA-10516.
-
Resolution: Fixed

> Implement Topic Command changes
> ---
>
> Key: KAFKA-10516
> URL: https://issues.apache.org/jira/browse/KAFKA-10516
> Project: Kafka
>  Issue Type: Sub-task
>Reporter: David Jacot
>Assignee: David Jacot
>Priority: Major
> Fix For: 2.7.0
>
>




--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Re: [DISCUSS] KIP-516: Topic Identifiers

[Ismael Juma]

Hi Lucas,

Why would you include the name and id? I think you'd want to remove the
name from the directory name right? Jason's suggestion was that if you
remove the name from the directory, then why would you need the id name
mapping file?

Ismael

On Thu, Sep 24, 2020 at 4:24 PM Lucas Bradstreet  wrote:

> > 2. Part of the usage of the file is to have persistent storage of the
> topic
> ID and use it to compare with the ID supplied in the LeaderAndIsr Request.
> There is some discussion in the KIP about changes to the directory
> structure, but I believe directory changes were considered to be out of
> scope when the KIP was written.
>
>
> Yeah, I was hoping to get a better understanding of why it was taken out of
> scope. Perhaps Lucas Bradstreet might have more insight about the decision.
> Basically my point is that we have to create additional infrastructure here
> to support the name/id mapping, so I wanted to understand if we just
> consider this a sort of tech debt. It would be useful to cover how we
> handle the case when this file gets corrupted. Seems like we just have to
> trust that it matches whatever the controller tells us and rewrite it?
>
>
> Hi Jason, Justine,
>
> My thought process is that we will likely need the metadata file whether we
> rename the directories or not.
> Linux supports filenames of up to 255 bytes and that would not be enough to
> support a directory name
>  including both the name and topic ID. Given that fact, it seemed
> reasonable to add the metadata file
> and leave the directory rename until some time in the future (possibly
> never).
>
> If the file does get corrupted I think you're right. We would either have
> to trust it matches what the controller tells us
>  or error out and let an administrator resolve it by checking across
> replicas for consistency.
>
> Lucas
>
>
> On Thu, Sep 24, 2020 at 3:41 PM Jason Gustafson 
> wrote:
>
> > Thanks Justine. Responses below:
> >
> > > 1. Yes, the directory will still be based on the topic names.
> > LeaderAndIsrRequest is one of the few requests that will still contain
> the
> > topic name. So I think we have this covered. Sorry for confusion.
> >
> > Ah, you're right. My eyes passed right over the field.
> >
> > > 2. Part of the usage of the file is to have persistent storage of the
> > topic
> > ID and use it to compare with the ID supplied in the LeaderAndIsr
> Request.
> > There is some discussion in the KIP about changes to the directory
> > structure, but I believe directory changes were considered to be out of
> > scope when the KIP was written.
> >
> > Yeah, I was hoping to get a better understanding of why it was taken out
> of
> > scope. Perhaps Lucas Bradstreet might have more insight about the
> decision.
> > Basically my point is that we have to create additional infrastructure
> here
> > to support the name/id mapping, so I wanted to understand if we just
> > consider this a sort of tech debt. It would be useful to cover how we
> > handle the case when this file gets corrupted. Seems like we just have to
> > trust that it matches whatever the controller tells us and rewrite it?
> >
> > > 3. I think this is a good point, but I again I wonder about the scope
> of
> > the KIP. Most of the changes mentioned in the KIP are for supporting
> topic
> > deletion and I believe that is why the produce request was listed under
> > future work.
> >
> > That's fair. I brought it up since `Fetch` is already included. If we've
> > got `Metadata` and `Fetch`, seems we may as well do `Produce` and save an
> > extra kip. No strong objection though if you want to leave it out.
> >
> >
> > -Jason
> >
> >
> > On Thu, Sep 24, 2020 at 3:26 PM Justine Olshan 
> > wrote:
> >
> > > Hi Jason,
> > >
> > > Thanks for your comments.
> > >
> > > 1. Yes, the directory will still be based on the topic names.
> > > LeaderAndIsrRequest is one of the few requests that will still contain
> > the
> > > topic name. So I think we have this covered. Sorry for confusion.
> > >
> > > 2. Part of the usage of the file is to have persistent storage of the
> > topic
> > > ID and use it to compare with the ID supplied in the LeaderAndIsr
> > Request.
> > > There is some discussion in the KIP about changes to the directory
> > > structure, but I believe directory changes were considered to be out of
> > > scope when the KIP was written.
> > >
> > > 3. I think this is a good point, but I again I wonder about the scope
> of
> > > the KIP. Most of the changes mentioned in the KIP are for supporting
> > topic
> > > deletion and I believe that is why the produce request was listed under
> > > future work.
> > >
> > > 4. This sounds like it might be a good solution, but I will need to
> > discuss
> > > more with KIP-500 folks to get the details right.
> > >
> > > Thanks,
> > > Justine
> > >
> > > On Thu, Sep 24, 2020 at 12:30 PM Jason Gustafson 
> > > wrote:
> > >
> > > > Hi Justine,
> > > >
> > > > Thanks for picking up this work. I have a few 

Re: [DISCUSS] KIP-673: Emit JSONs with new auto-generated schema

[Tom Bentley]

Hi Anastasia,

Thanks for the KIP, I can certainly see the benefit of this. I have a few
questions:

1. I think it would be helpful to readers to explicitly illustrate the
RequestConvertToJson and XYZJsonDataConverter classes (e.g. with method
signatures for one or two methods), because currently it's not clear (to me
at least) exactly what's being proposed. Does the RequestConvertToJson
return a XYZJsonDataConverter?

2. Does the serialization have a defined order of properties (alphabetic
perhaps)? My concern here is that properties appearing in order according
to how they are iterated in a hash map might harm human readability of the
logs.

3. Would the serialization be done via the Jackson databinding?

Many thanks,

Tom

On Thu, Sep 24, 2020 at 11:49 PM Anastasia Vela  wrote:

> Hi all,
>
> I'd like to discuss KIP-673:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-673%3A+Emit+JSONs+with+new+auto-generated+schema
>
> This is a proposal to change the format of request and response traces to
> JSON, which would be easier to load and parse, because the current format
> is only JSON-like and not easily parsable.
>
> Let me know what you think,
> Anastasia
>

Re: Question Regarding Offset Behavior When Calling Poll()

[Zhen Zhang]

Hi,

Sorry for the late reply, let me clarify on this.

I am developing using Golang so I used a library based on librdkafka, and
there's one function, ReadMesage(), which is a wrapper on top of the poll()
function, except that it will only poll one message(record) at a time and
return either one of the following,

1. (msg, nil) -> normal situation with no error;
2. (nil, err) -> err is a Kafka timeout error;
3. (nil, err) -> err is general error;
4. (msg, err) -> err is partition-specific error.

When I was browsing the javadocs
https://kafka.apache.org/26/javadoc/index.html?org/apache/kafka/clients/consumer/KafkaConsumer.html,
I noticed the concept of `Offsets and Consumer Position`, so I am wondering
that in the event of the situation 2, 3 and 4, will the position still
increment by 1 even if what I get from ReadMessage() is an error?

I tried to read the docs but didn't find anything useful I can relate to, I
tried to ask for help from the contributors of the library I was using but
didn't get an answer, not sure if this question is too obvious or too
noobie lolol. Since that library I used is an implementation of the Kafka
protocol, I decided to ask it here and I sincerely hope I can get some
insights from a Kafka guru.

Thanks,
Zhen Zhang
Software Engineer
[image: Twilio] 
MOBILE (949) 771-6073
EMAIL zzh...@twilio.com


On Wed, Sep 23, 2020 at 9:45 AM Matthias J. Sax  wrote:

> I guess it depends where the exception comes from? Can you clarify?
>
> -Matthias
>
> On 9/23/20 12:53 AM, Zhen Zhang wrote:
> > Hi there,
> >
> > I am new to Kafka and I would like to get some clarifications for a
> newbie
> > question,
> >
> > Let's say if I have set up my consumer's "enable.auto.commit" to false,
> and
> > then poll the records one at a time. So when calling poll(), starting
> from
> > offset 0, if any exception is thrown, should I expect to get the record
> at
> > offset 0 or offset 1 when I call poll() again? The reason I'm asking for
> > this is bc in the Kafka Doc, it says that,
> > "The position of the consumer gives the offset of the next record that
> will
> > be given out. It will be one larger than the highest offset the consumer
> > has seen in that partition. It automatically advances every time the
> > consumer receives messages in a call to poll(Duration)."
> >
> > But in my described situation above, an exception is thrown, I'm not sure
> > if this is counted as a successful poll (meaning that the next poll()
> will
> > give the next record) or a failed one (meaning that the next poll() will
> > give the same record again).
> >
> > I would really appreciate it for your help.
> >
> > Thanks,
> > Zhen Zhang
> > Software Engineer
> > [image: Twilio] 
> > MOBILE (949) 771-6073
> > EMAIL zzh...@twilio.com
> >
>

Can I get a review for a documentation update (KAFKA-10473)?

[James Cheng]

Hi,

Can I get a review from one of the commiters for this documentation update?

I am adding docs for the following JMX metrics:
kafka.log,type=Log,name=Size
kafka.log,type=Log,name=NumLogSegments
kafka.log,type=Log,name=LogStartOffset
kafka.log,type=Log,name=LogEndOffset


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

https://github.com/apache/kafka/pull/9276 


The pull request page lists lots of failed checks. However, this pull request 
only modifies an HTML file, and the test failures don't seem related to my 
changes.

Thanks,
-James