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

[fix][py] Correct configuration of pattern discovery period #17912

Closed
wants to merge 1 commit into from
Closed

[fix][py] Correct configuration of pattern discovery period #17912

wants to merge 1 commit into from

Conversation

sbreatnach
Copy link

Motivation

Currently, the pattern_auto_discovery_period for topic pattern consumers is fixed to 60 seconds, no matter what is passed into the subscribe function. This is a fix for that bug.

Modifications

Pattern discovery period configuration is now passed through correctly from the subscribe function.
New test to exercise pattern discovery.
Removed misleading configuration in other test which does not use automated pattern discovery.
Made log level configurable in test file with environment variable.

If the changes are ok, I can backport these changes to earlier branches - we use v2.9 currently so I have a specific requirement for that version. I can also open a PR against the new v3 repository if required.

Verifying this change

  • Make sure that the change passes the CI checks.

    • A new integration test for the Python client library has been added
    • An existing test was slightly changed to remove misleading comments

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    Documentation now matches behaviour described for the pattern_auto_discovery_period option.

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Matching PR in forked repository

PR in forked repository:
Successful manual run of CI job in forked repository

New test to exercise pattern discovery.
Removed misleading configuration in other test which does not use automated pattern discovery.
Made log level configurable in test with environment variable.
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 3, 2022
@BewareMyPower
Copy link
Contributor

The Python client has been moved out of the main repo now, please open a PR here: https://github.com/apache/pulsar-client-python

@sbreatnach
Copy link
Author

sbreatnach commented Oct 4, 2022

@BewareMyPower how do I get this fix into any of the v2 versions of the client if I open it against the new repo? My understanding is that repo is for v3+ only. According to the PIP, bug fixes are still allowed for existing branches.

@BewareMyPower BewareMyPower reopened this Oct 4, 2022
@BewareMyPower
Copy link
Contributor

@sbreatnach Yes. But currently the commits to existing branches cannot trigger the CI for Python tests. Though currently the CI in the new repo is not ready at the moment. IMO,

I reopened this PR just now, but I think the correct process is:

  1. Open a PR in https://github.com/apache/pulsar-client-python, which should trigger the CI tests.
  2. If the PR is a bug fix, cherry-pick it back into the existing branch-2.x branches.

WDYT? @merlimat

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Oct 4, 2022

Keeping this PR open might lead some committers to click the "Squash and merge" button to merge this PR without any passed CI.

image

So I closed this PR again until getting the response from @merlimat

@merlimat
Copy link
Contributor

merlimat commented Oct 4, 2022

Yes, @sbreatnach please open the PR in the new repo and once it gets merged, the committers will port it back to the older branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants