Skip to content

Conversation

junhao69535
Copy link

Description

Previously, only patch __next__ was used, which caused poll to fail. The __next__ method actually calls the poll method, so the patch poll method is used instead.

    def poll(self, timeout_ms=0, max_records=None, update_offsets=True):
        assert timeout_ms >= 0, 'Timeout must not be negative'
        if max_records is None:
            max_records = self.config['max_poll_records']
        assert isinstance(max_records, int), 'max_records must be an integer'
        assert max_records > 0, 'max_records must be positive'
        assert not self._closed, 'KafkaConsumer is closed'

        timer = Timer(timeout_ms)
        while not self._closed:
            records = self._poll_once(timer, max_records,
                                      update_offsets=update_offsets)
            if records:
                return records
            elif timer.expired:
                break
        return {}
    
    def _message_generator_v2(self):
        timeout_ms = 1000 * max(0, self._consumer_timeout - time.time())
        record_map = self.poll(timeout_ms=timeout_ms, update_offsets=False)
        for tp, records in six.iteritems(record_map):
            for record in records:
                if not self._subscription.is_fetchable(tp):
                    log.debug("Not returning fetched records for partition %s"
                              " since it is no longer fetchable", tp)
                    break
                self._subscription.assignment[tp].position = OffsetAndMetadata(record.offset + 1, '', -1)
                yield record

    def __next__(self):
        if self._closed:
            raise StopIteration('KafkaConsumer closed')
        self._set_consumer_timeout()
        while time.time() < self._consumer_timeout:
            if not self._iterator:
                self._iterator = self._message_generator_v2()
            try:
                return next(self._iterator)
            except StopIteration:
                self._iterator = None
        raise StopIteration()

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Add a new test at opentelemetry.instrumentation.kafka.tests.test_utils.test_wrap_poll.

  • opentelemetry.instrumentation.kafka.tests.test_utils.test_wrap_poll

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Copy link

linux-foundation-easycla bot commented Aug 6, 2025

CLA Signed

  • ✅login: junhao69535 / name: Jun Hao / (9da04aa)

The committers listed above are authorized under a signed CLA.

@junhao69535 junhao69535 force-pushed the feature/replace_kafka_python_patch branch from 5bfdf96 to 9da04aa Compare August 6, 2025 12:09
@xrmx xrmx moved this to Reviewed PR that needs fixing in @xrmx's Python PR digest Aug 22, 2025
@xrmx
Copy link
Contributor

xrmx commented Aug 22, 2025

@junhao69535 Thanks for the PR but you need to sign the CLA in order to contribute to the project.

@junhao69535
Copy link
Author

@junhao69535 Thanks for the PR but you need to sign the CLA in order to contribute to the project.

ok, I've finished.

@xrmx xrmx moved this from Reviewed PR that needs fixing to Ready for review in @xrmx's Python PR digest Aug 28, 2025
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

@junhao69535 Have you checked if we have a test that actually test some kafka usage or is it just unit testing our helpers? Anyway would be nice to add something in /tests/opentelemetry-docker-tests/tests because at the moment we have no way to check that the behaviour hasn't changed

@junhao69535
Copy link
Author

@junhao69535 Have you checked if we have a test that actually test some kafka usage or is it just unit testing our helpers? Anyway would be nice to add something in /tests/opentelemetry-docker-tests/tests because at the moment we have no way to check that the behaviour hasn't changed

yes, we have a test that actually test some kafka usage and use in prod env.

@junhao69535
Copy link
Author

@xrmx hi, are there any other questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

2 participants