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

Added a delete records api #1710

Merged
merged 49 commits into from
Jul 9, 2024
Merged

Added a delete records api #1710

merged 49 commits into from
Jul 9, 2024

Conversation

PratRanj07
Copy link
Contributor

Added a delete records api and created integration tests and unit tests for the same

@PratRanj07 PratRanj07 requested a review from pranavrth February 14, 2024 12:21
@PratRanj07 PratRanj07 requested a review from a team as a code owner February 14, 2024 12:21
Copy link

cla-assistant bot commented Feb 14, 2024

CLA assistant check
All committers have signed the CLA.

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.

Reviewed Halfway. Still reviewing. Please resolve these issues.

examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
@PratRanj07 PratRanj07 requested a review from pranavrth February 29, 2024 05:50
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.

One more pass.

examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
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.

Some more review comments.

src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

I have these concerns for the public API that will we frozen after we release it

examples/adminapi.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
const rd_kafka_DeleteRecords_result_t *c_delete_records_res = rd_kafka_event_DeleteRecords_result(rkev);
const rd_kafka_topic_partition_list_t *c_delete_records_res_list = rd_kafka_DeleteRecords_result_offsets(c_delete_records_res);

result = c_parts_to_py(c_delete_records_res_list);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the responses are returned in the same order as the requests, so that it's possible to use _make_futmap_result_from_list

Also we should use a DeletedRecords object as in Java, instead of a TopicPartition because that could be extended in future and in that case we cannot apply the same changes.

Copy link
Member

Choose a reason for hiding this comment

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

We are converting it to the futmap in the end keyed at TopicPartition so there is no issue in that I think.

We can create DeletedRecords class as Java instead of directly using offset in the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a new class DeleteRecords containing only offset and made changes according to that. Please review.

@PratRanj07 PratRanj07 requested review from pranavrth and emasab May 2, 2024 05:32
CHANGELOG.md Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/_model/__init__.py Outdated Show resolved Hide resolved
@PratRanj07 PratRanj07 requested a review from pranavrth May 27, 2024 10:36
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
@PratRanj07 PratRanj07 requested a review from pranavrth May 27, 2024 13:08
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.

Some more fixes.

src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
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.

Great work. LGTM!..

@pranavrth pranavrth requested a review from a team as a code owner July 5, 2024 14:58
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Here are my proposed changes

examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
tests/integration/admin/test_delete_records.py Outdated Show resolved Hide resolved

param list(TopicPartitions) topic_partition_offsets: A list of
TopicPartition objects having `offset` field set to the offset
before which all the records should be deleted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
before which all the records should be deleted.
before which all the records should be deleted.
`offset` can be set to :py:const:`OFFSET_END` (-1) to delete all records
in the partition.

Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Comments about SetItem error handling, c_brokers_to_py and error format

src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Some comments about the sphinx documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

Add DeletedRecords to index.rst after ConsumerGroupDescription

- :ref:`DeletedRecords <pythonclient_deleted_records>`

and

.. _pythonclient_deleted_records:

**************
DeletedRecords
**************

.. autoclass:: confluent_kafka.admin.DeletedRecords
   :members:

in the specified Topic and Partitions.

:param list(TopicPartitions) topic_partition_offsets: A list of
TopicPartition objects having `offset` field set to the offset
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TopicPartition objects having `offset` field set to the offset
:class:`.TopicPartition` objects having `offset` field set to the offset

including broker lookup, request transmission, operation time
on broker, and response. Default: `socket.timeout.ms*1000.0`
:param float operation_timeout: The operation timeout in seconds,
controlling how long the delete_records request will block
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
controlling how long the delete_records request will block
controlling how long the `delete_records` request will block

in the cluster. A value of 0 returns immediately.
Default: `socket.timeout.ms/1000.0`

:returns: A dict of futures keyed by the TopicPartition.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:returns: A dict of futures keyed by the TopicPartition.
:returns: A dict of futures keyed by the :class:`.TopicPartition`.

Default: `socket.timeout.ms/1000.0`

:returns: A dict of futures keyed by the TopicPartition.
The future result() method returns DeletedRecords
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The future result() method returns DeletedRecords
The future result() method returns :class:`.DeletedRecords`


:returns: A dict of futures keyed by the TopicPartition.
The future result() method returns DeletedRecords
or raises KafkaException
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
or raises KafkaException
or raises :class:`.KafkaException`

def delete_records(self, topic_partition_offsets, **kwargs):
"""
Deletes all the records before the specified offsets (not including),
in the specified Topic and Partitions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Written this way they seem like classes

Suggested change
in the specified Topic and Partitions.
in the specified topics and partitions.

Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Great work! Thanks @PratRanj07 and @pranavrth

@pranavrth pranavrth merged commit 65ab14c into master Jul 9, 2024
2 checks passed
@pranavrth pranavrth deleted the deleterecords branch July 9, 2024 14:08
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