Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KIP-848] User documentation #4702

Merged
merged 5 commits into from
May 7, 2024
Merged

[KIP-848] User documentation #4702

merged 5 commits into from
May 7, 2024

Conversation

emasab
Copy link
Contributor

@emasab emasab commented Apr 29, 2024

Added changelog entry and a section in INTRODUCTION.md

@emasab emasab force-pushed the dev_kip848_documentation branch 3 times, most recently from 532c7fc to b158759 Compare April 29, 2024 17:37
@emasab emasab force-pushed the dev_kip848_documentation branch from b158759 to d440241 Compare April 30, 2024 14:28
CHANGELOG.md Outdated
@@ -32,6 +33,9 @@ librdkafka v2.4.0 is a feature release:
error. Rest of records in the batch will fail with the new error code
_INVALID_DIFFERENT_RECORD (Java: KafkaException) and can be retried manually,
depending on the application logic (#4583).
* The new consumer group rebalance protocol, defined in KIP 848, is still in **Early Availability**: _not production-ready_, _not supported_.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we usually refer to it as "Early Access", and that't actually the term we see in the logs when running the broker with it...

The new 'consumer' rebalance protocol is enabled along with the new group coordinator. This is part of the early access of KIP-848 and MUST NOT be used in production

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also add a warning log for the same.

Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First Pass

CHANGELOG.md Outdated
@@ -2,7 +2,11 @@

librdkafka v2.4.0 is a feature release:

* [KIP-467](https://cwiki.apache.org/confluence/display/KAFKA/KIP-467%3A+Augment+ProduceResponse+error+messaging+for+specific+culprit+records) Augment ProduceResponse error messaging for specific culprit records (#4583).
* [KIP-848 EA](https://cwiki.apache.org/confluence/display/KAFKA/KIP-848%3A+The+Next+Generation+of+the+Consumer+Rebalance+Protocol): The Next Generation of the Consumer Rebalance Protocol (#4610)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this EA outside of the brackets and it should be spelled fully "EARLY ACCESS".

We should explain little more here itself instead of below. Something like "EARLY ACCESS: This should be used only for testing and not to be used in Production. Some of the feature/contract of this KIP might be changed in future."

CHANGELOG.md Outdated
Comment on lines 36 to 38
* The new consumer group rebalance protocol, defined in KIP 848, is still in **Early Access**: _not production-ready_, _not supported_.
It's possible to try it in a non-production enviroment.
A [guide](INTRODUCTION.md#next-generation-of-the-consumer-group-protocol-kip-848) is available (#4610).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a separate section for KIP-848 in the CHANGELOG I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a section for Early Access features separate from Upgrade considerations

INTRODUCTION.md Outdated
Comment on lines 1546 to 1548
Starting from librdkafka 2.4.0 the next generation consumer group rebalance protocol
is in **Early Access**. It means it's still _not production-ready_ and
_not supported_, given it's still under validation and lacking some needed features.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Starting from librdkafka 2.4.0 the next generation consumer group rebalance protocol
is in **Early Access**. It means it's still _not production-ready_ and
_not supported_, given it's still under validation and lacking some needed features.
Starting from librdkafka 2.4.0 the next generation consumer group rebalance protocol introduced in KIP-848
is introduced.
Warning: It is still in **Early Access** which means it's still _not production-ready_ and
_not supported_, given it's still under validation and lacking some needed features. Some
of the feature and contracts might change in future.

INTRODUCTION.md Outdated
Comment on lines 1554 to 1556
To test it, a Kafka cluster must be set up, in KRaft mode, and the new group
protocol enabled with the `group.coordinator.rebalance.protocols` property.
Broker version must be Apache Kafka 3.7.0 or newer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INTRODUCTION.md Outdated
Comment on lines 1575 to 1576
- Client side assignors as described in the KIP
- Online upgrade of a non-empty consumer group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we should call these two as missing features or not. Adding them to missing features shows that we are planning to implement these in future which we are not as of now.

Instead of "Missing Features" we should call them "Future features" and remove these two points from there.

INTRODUCTION.md Outdated
Comment on lines 1615 to 1619
For the same reason, when closing or unsubscribing with auto-commit set,
the member will try to commit until a specific timeout has passed.
Currently the timeout is the same as the `classic` protocol and it corresponds
to the deprecated `session.timeout.ms`, but it will change before the feature
reaches a stable state.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should explain that we are still using deprecated property.

INTRODUCTION.md Outdated
Comment on lines 1621 to 1623
- An `UNKNOWN_TOPIC_OR_PART` error won't be received when a consumer is
subscribing to a topic that doesn't exist in local cache, as the consumer
is still subscribing to the topic and it could be created just after that.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explain more on why we are writing this. What used to happen in old protocol.

INTRODUCTION.md Outdated
Comment on lines 1625 to 1627
- A consumer won't do a preliminary Metadata call that returns a
`TOPIC_AUTHORIZATION_FAILED`, topic partitions will be assigned to the member
by the Coordinator only if it's authorized to consume from the topic.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

INTRODUCTION.md Outdated
Comment on lines 1629 to 1630
- Number of assign/revoke callbacks isn't fixed anymore, as it depends on
heartbeat timing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean and does it affect the users?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to adjust few sections and move some part to the changelog instead of here.

@emasab emasab force-pushed the dev_kip848_documentation branch 2 times, most recently from 2f20f13 to c053c06 Compare May 6, 2024 16:38
@emasab emasab force-pushed the dev_kip848_documentation branch from c053c06 to dc886dd Compare May 6, 2024 16:41
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Provided minor comments.

@@ -34,6 +36,18 @@ librdkafka v2.4.0 is a feature release:
depending on the application logic (#4583).


## Early Access
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its better to put KIP-848 related section in itself so that we emphasize that this is a major feature we care about.

CHANGELOG.md Outdated
Comment on lines 40 to 45
* The new consumer group rebalance protocol, defined in KIP 848, is still _not production-ready_ and _not supported_.
It's possible to try it in a non-production enviroment.

With this protocol the role of the Group Leader (a member) is removed and
the assignment is calculated by the Group Coordinator (a broker) and sent
to each member through heartbeats.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these two paragraph can be swapped. We want to explain about the protocol first and then tell about the caveats.

@emasab emasab force-pushed the dev_kip848_documentation branch from 2217907 to 4bc0cdf Compare May 6, 2024 22:54
CHANGELOG.md Outdated
the assignment is calculated by the Group Coordinator (a broker) and sent
to each member through heartbeats.

The feature is still _not production-ready_ and _not supported_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean by KIP-848 is not supported? I think we should explain.

Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!.

@emasab emasab merged commit 18bc849 into master May 7, 2024
2 checks passed
@emasab emasab deleted the dev_kip848_documentation branch May 7, 2024 08:40
anchitj pushed a commit that referenced this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants