[KIP-848] Mock handler and Integration tests passing by emasab · Pull Request #4662 · confluentinc/librdkafka

Emanuele Sabellico (emasab) · 2024-03-26T17:40:27Z

Mock handler implementation
Rename current consumer protocol from generic to classic
Mock handler with automatic or manual assignment
More consumer group metadata getters
Test helpers
Expedite next HB after FindCoordinator
doing it with an exponential backoff to avoid tight loops
Configurable session timeout and HB interval
Fix mock handler ListOffsets response
LeaderEpoch instead of CurrentLeaderEpoch
Integration tests passing with AK trunk
Improve documentation and KIP 848 specific mock tests

Pranav Rathi (pranavrth)

For src files.

Pranav Rathi (pranavrth) · 2024-07-15T13:59:25Z

+/**
+ * @brief Get member epoch of a group metadata.
+ *        Corresponds to the generation id in consumer protocol classic;
+ *
+ * @param group_metadata The group metadata
+ *
+ * @returns The member epoch id contained in the passed \p group_metadata.
+ */
+RD_EXPORT
+int32_t rd_kafka_consumer_group_metadata_member_epoch(
+    const rd_kafka_consumer_group_metadata_t *group_metadata);


Do we have method for retrieving geenration id? If not, do we need one for member epoch? If we really need this for member epoch, let's create one for generation id separately.

In Java GroupMetadata they haven't added a new one for member epoch, maybe naming can be changed in a future major release.

I was saying that shall we add a new method rd_kafka_consumer_group_metadata_generation_id as well.

OK, I think I'll leave only rd_kafka_consumer_group_metadata_generation_id as they didn't add the memberEpoch method in Java

/** * @brief Get the generation id (classic protocol) * or member epoch (consumer protocol) of a group

Is this fine description?

Yes, correct

Pranav Rathi (pranavrth) · 2024-07-17T14:06:34Z

+        rd_kafka_buf_read_str(rkbuf, &ServerAssignor);
+
+        /* #TopicPartitions */
+        rd_kafka_buf_read_arraycnt(rkbuf, &TopicPartitionsCnt,


I think we should use rd_kafka_buf_read_topic_partitions here.

If you do that, then you need to scan the topic partition list, and for each topic partition you need to compare topic id to see if it has changed to call rd_kafka_mock_topic_find_by_id. If it returned like a Map<Topic, List<Partition>> or we didn't have to do an operation at each topic change I'd be more inclined to do than. What's your view on this?

I think its better to use 2 loops here. First to read the data from the rpc and second for our business logic. This ensures readability, reusability and separation of concern.

Pranav Rathi (pranavrth)

Some 14 files more remaining but the main one is tests/0147-consumer_group_consumer_mock.c.

Pranav Rathi (pranavrth) · 2024-07-18T13:30:39Z

-        do_test_CreateTopics("temp queue, op timeout 0", rk, NULL, 0, 0);
+        /* FIXME: KRaft async CreateTopics is working differently than
+         * wth Zookeeper
+         * do_test_CreateTopics("temp queue, op timeout 0", rk, NULL, 0, 0); */


Does AK team know about it?

Let's have another function to determine whether its a zookeeper or kraft cluster and disable only when it kraft.

Sure, I opened this issue.
Then talked about it internally. I think it was a bug and not a feature given they didn't acknowledge it was a feature to replicate with KRaft, we could remove the test for both when that is closed.

Pranav Rathi (pranavrth) · 2024-07-18T13:52:12Z

+      if (test_consumer_group_protocol_classic() &&
          !(rebalance_cb1.assign_call_cnt == expected_cb1_assign_call_cnt &&
            rebalance_cb2.assign_call_cnt == expected_cb2_assign_call_cnt))


We should have similar calculations for the new protocol as well.

Same for others as well below.

With KIP-848 the callback count isn't fixed as in classic protocol so we can only verify the expected final assignment but not the number of callbacks.

I think we can do that deterministically but might be more complex.

Pranav Rathi (pranavrth) · 2024-07-18T14:12:59Z

+  } else if (0) {
+    /* FIXME: enable this once new errors are handled in OffsetCommit. */
+    t_consumer_group_consumer_retry_commit_on_fenced_member_epoch();
+  }


I will enable this in my other opened PR.

I checked it and I think we should remove this test, as now OffsetCommit returns STALE_MEMBER_EPOCH that is not retried automatically and FENCED _MEMBER_EPOCH is returned by the Heartbeat only

Pranav Rathi (pranavrth) · 2024-07-18T14:18:59Z

+        if (test_consumer_group_protocol_classic()) {
+                ApiKey = RD_KAFKAP_Heartbeat;
+        } else {
+                ApiKey = RD_KAFKAP_ConsumerGroupHeartbeat;
+        }


This should be opposite condition. if condition should be for consumer as that is the new one.

I've always used always test_consumer_group_protocol_classic(), even if implementing the other function (that I can remove) so it's consistent for search, also it isolates the parts that we want to remove in a distant future.
Also if there was a third protocol, we hope not, that should start from the assumptions and tests of the consumer protocol, not with those of the classic protocol.

We should be using test_consumer_group_protocol_consumer at all the places instead of test_consumer_group_protocol_classic. Reason behind this is:

Let's say we add a new protocol in future. We want all the tests to run. We are making sure of that by using !test_consumer_group_protocol_consumer() here instead.

In !test_consumer_group_protocol_classic() condition, we will skip those tests with the new protocol added.

No, in this case we're running the RD_KAFKAP_ConsumerGroupHeartbeat test that is more appropriate than running the classic one, if we use test_consumer_group_protocol_consumer ... else in case of a new protocol we run the classic tests, while running the newer ones would be more appropriate.

I am talking about all the places. Not just here. Basically the idea is to make sure that we run all the test cases when we add new protocols. Let's ensure that when there is a new protocol, all the tests are executed.

yes, all tests about the new protocol, the tests about the classic protocol don't have to be executed as they won't be applicable. Probably the tests about the 848 protocol won't be applicable either but it's more probable it'll be similar to 848 in case it happens.

Pranav Rathi (pranavrth)

Some more comments

Pranav Rathi (pranavrth) · 2024-07-25T10:33:50Z

We should release new trivup version. Lets check with Magnus.

confluent-cla-assistant · 2024-10-03T19:00:53Z

🎉 All Contributor License Agreements have been signed. Ready to merge.
_{Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.}

Pranav Rathi (pranavrth) · 2024-11-14T10:38:24Z

+                        } else {
+                                rd_kafka_mock_cgrp_consumer_member_leave(
+                                    mcgrp, member);
+                                member = NULL;
+                        }


In the static membership case (-2), the member can rejoin. We shouldn't destroy the member. You can add a FIXME for now here.

I've implemented it and added some tests for static group membership with the mock cluster

Pranav Rathi (pranavrth) · 2024-11-18T07:14:37Z

+            - make -j -C tests build
+            - make -C tests run_local_quick
+            - DESTDIR="$PWD/dest" make install
+            - export TEST_KAFKA_VERSION=4.0.0


Where is this being used? 4.0.0 is not released yet.

This is used in tests to check if a test can run or not, if for example it's >= a specific version, given we're using master I used the next planned release here, but could have used 9.9.9 too

- Mock handler implementation - Rename current consumer protocol from generic to classic - Mock handler with automatic or manual assignment - More consumer group metadata getters - Test helpers - Expedite next HB after FindCoordinator doing it with an exponential backoff to avoid tight loops - Configurable session timeout and HB interval - Fix mock handler ListOffsets response LeaderEpoch instead of CurrentLeaderEpoch - Integration tests passing with AK trunk - Improve documentation and KIP 848 specific mock tests - Add mock tests for unknown topic id in metadata request and partial reconciliation - Make test 0147 more reliable - Fix test 0106 after HB timeout change - Exclude test case with AK trunk - Rename rd_kafka_buf_write_tags to rd_kafka_buf_write_tags_empty - Trivup 0.12.5 can run a KafkaCluster directly with KRaft and AK trunk - Trivup 0.12.6 build with a specific commit

and Py 3.12

an issue with apache/kafka#16464 on AK > 3.8.0

Address comment about using rd_kafka_buf_read_topic_partitions Asserts on non-nullable fields

…ava client

…ent_new

Pranav Rathi (pranavrth)

LGTM!.

Pranav Rathi (pranavrth)

LGTM!

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from 6667956 to ca1eb2d Compare March 26, 2024 17:48

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch 2 times, most recently from 1d9962c to ca5e6d0 Compare March 27, 2024 10:43

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from ca1eb2d to 953f668 Compare March 28, 2024 14:41

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch 6 times, most recently from fd13d77 to f519c11 Compare March 28, 2024 22:35

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from 953f668 to e4614be Compare April 1, 2024 14:40

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch from f519c11 to 029b5ab Compare April 1, 2024 14:41

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from e4614be to 10e7cc1 Compare April 1, 2024 15:07

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch from 029b5ab to d7428d0 Compare April 1, 2024 15:07

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from 10e7cc1 to 5789307 Compare April 1, 2024 16:48

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch from d7428d0 to 0379129 Compare April 1, 2024 16:50

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch 2 times, most recently from 434da75 to e5f3101 Compare April 3, 2024 13:40

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch from 0379129 to c6c9d2e Compare April 3, 2024 13:41

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from e5f3101 to 109d348 Compare April 3, 2024 16:54

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch from c6c9d2e to cf1ad35 Compare April 3, 2024 16:54

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from 109d348 to 3dd0e45 Compare April 5, 2024 08:28

Emanuele Sabellico (emasab) changed the base branch from dev_kip848_use_metadata_cache_and_fixes to master April 5, 2024 08:38

Emanuele Sabellico (emasab) changed the base branch from master to dev_kip848_use_metadata_cache_and_fixes April 5, 2024 08:39

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from 3dd0e45 to 3bba91f Compare April 5, 2024 19:19

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch from 0396b1f to 27817d0 Compare April 5, 2024 19:21

Emanuele Sabellico (emasab) force-pushed the dev_kip848_use_metadata_cache_and_fixes branch from 3bba91f to f4fb7a1 Compare April 5, 2024 19:52

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch 3 times, most recently from 91df086 to 82774f9 Compare April 6, 2024 12:14

Base automatically changed from dev_kip848 to master April 18, 2024 11:03

Emanuele Sabellico (emasab) mentioned this pull request Apr 18, 2024

Fix hang in incremental assign during close #4528

Merged

Emanuele Sabellico (emasab) force-pushed the dev_kip848_mock_handler_and_integration_tests branch 2 times, most recently from f04dc32 to 8d9ed36 Compare April 24, 2024 15:48

mahajanadhitya reviewed Jul 1, 2024

View reviewed changes

Comment thread src/rdkafka_mock_handlers.c Outdated

Pranav Rathi (pranavrth) requested changes Jul 17, 2024

View reviewed changes

Pranav Rathi (pranavrth) requested changes Jul 18, 2024

View reviewed changes

Pranav Rathi (pranavrth) reviewed Jul 25, 2024

View reviewed changes

Pranav Rathi (pranavrth) reviewed Nov 14, 2024

View reviewed changes

Pranav Rathi (pranavrth) reviewed Nov 15, 2024

View reviewed changes

Comment thread tests/0120-asymmetric_subscription.c

Pranav Rathi (pranavrth) reviewed Nov 15, 2024

View reviewed changes

Comment thread tests/0147-consumer_group_consumer_mock.c Outdated

Pranav Rathi (pranavrth) requested changes Nov 20, 2024

View reviewed changes

Emanuele Sabellico (emasab) added 13 commits December 13, 2024 12:41

trivup 0.12.7 with fixes for AK 3.8.0

8f02e16

and Py 3.12

New version of trivup 0.12.7 to fix

182455d

an issue with apache/kafka#16464 on AK > 3.8.0

Address comments

40275ff

Static group membership mock tests

c38588a

Address comment about using rd_kafka_buf_read_topic_partitions Asserts on non-nullable fields

Leave only the rd_kafka_consumer_group_metadata_generation_id as in J…

a02340b

…ava client

Rename simple mock assignment

9196720

Rename mock configuration setters

6a4d6f9

Change parameters order in rd_kafka_mock_cgrp_consumer_target_assignm…

c83bb53

…ent_new

Rename to assignment_to_return

b2a00d2

Test the range assignor with 848 mock cluster too

34bf478

Variable naming

66f7095

Move test 0147 to a different PR

2fbf74c

Pranav Rathi (pranavrth) approved these changes Dec 13, 2024

View reviewed changes

Disable interactive needs restart prompt

48c3859

Pranav Rathi (pranavrth) approved these changes Dec 13, 2024

View reviewed changes

Conversation

Emanuele Sabellico (emasab) commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Pranav Rathi (pranavrth) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emanuele Sabellico (emasab) Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pranav Rathi (pranavrth) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emanuele Sabellico (emasab) Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emanuele Sabellico (emasab) Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emanuele Sabellico (emasab) commented Mar 26, 2024 •

edited

Loading

Emanuele Sabellico (emasab) Nov 21, 2024 •

edited

Loading

Emanuele Sabellico (emasab) Oct 28, 2024 •

edited

Loading

Emanuele Sabellico (emasab) Nov 21, 2024 •

edited

Loading