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

[client][python] Add getLastMessageIdAsync C binding #16255

Merged
merged 7 commits into from
Aug 9, 2022

Conversation

komalatammal
Copy link
Contributor

@komalatammal komalatammal commented Jun 28, 2022

Motivation

Python function getLastMessageId

It is a C binding for #16182 to implement get_last_message_id() in Python client.

Modifications

Add Python/C binding code for get_last_message_id()

Verifying this change

It compiles.

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (yes)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly.

Need to update docs?

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

  • doc-not-needed

  • doc
    Python Doc is updated in init.py

  • doc-complete
    (Docs have been already added)

@github-actions
Copy link

@komalatammal Please select only one documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jun 28, 2022
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Could you add a test to pulsar_test.py?

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

@michaeljmarshall michaeljmarshall merged commit fd8ebaa into apache:master Aug 9, 2022
@michaeljmarshall
Copy link
Member

@komalatammal @eolivelli @BewareMyPower - it'd be nice to cherry pick this to 2.11 since it is a feature that is already in the Java client api. Do you think it is too late to get this change into that branch?

@BewareMyPower
Copy link
Contributor

@michaeljmarshall I think it needs a discussion. There was a similar discussion here: #15822 (comment) /cc @nodece

The core question is that should we cherry-pick all these new features of C++/Python clients to release branches?

My personal answer is no. The feature catch up should not be cherry-picked. For the same reason, if users want some new features of a specific component, they have to upgrade the major version.

Regarding 2.11.0 release, I think you can reply in the mail list. I see branch-2.11 has already been created.

@michaeljmarshall
Copy link
Member

My personal answer is no. The feature catch up should not be cherry-picked. For the same reason, if users want some new features of a specific component, they have to upgrade the major version.

I agree with this line of thinking. My only reason for asking in this case is because branch-2.11 was only just recently created and the RC has not yet been tagged. I am not going to ask on the ML because I'll just let this target 2.12.

@michaeljmarshall michaeljmarshall added this to the 2.12.0 milestone Aug 9, 2022
@nodece
Copy link
Member

nodece commented Aug 9, 2022

@BewareMyPower @michaeljmarshall Thanks for your explanation! I have a question about the broker and client of old version, if the broker supports the xx feature, but the client doesn't support it, shouldn't we cherry-pick it?

@BewareMyPower
Copy link
Contributor

if the broker supports the xx feature, but the client doesn't support it

IMO no as I've said:

The feature catch up should not be cherry-picked.

@michaeljmarshall
Copy link
Member

@BewareMyPower @michaeljmarshall Thanks for your explanation! I have a question about the broker and client of old version, if the broker supports the xx feature, but the client doesn't support it, shouldn't we cherry-pick it?

One reason I think it is unnecessary to backport features to older versions of the client is that the protocol has a feature handshake built in that is supposed to enable different versions of the clients and servers to work together seamlessly. Therefore, if a user wants to get a new feature in the client that was already supported on the broker in an older version, they should be able to upgrade without issue. That being said, I am not sure that we document the client/server compatibility anywhere and I am not sure how good the test coverage is for the many permutations of client versions.

@nodece
Copy link
Member

nodece commented Aug 9, 2022

Ok, I see.

@Technoboy- Technoboy- modified the milestones: 2.12.0, 2.11.0 Aug 10, 2022
Technoboy- pushed a commit that referenced this pull request Aug 10, 2022
* python cc binding for getLastMessageId

* add python Consumer class method and doc

* fix linter issues based on clang-format

* ubuntu linter fix

* try run unit test in ci

* fix doc comment

* test the test case can be ran

### Motivation

Python function getLastMessageId

It is a C binding for #16182 to implement get_last_message_id() in Python client.

### Modifications

Add Python/C binding code for get_last_message_id()

### Verifying this change

It compiles.
- [x] Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.


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

*If `yes` was chosen, please highlight the changes*

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API: (yes)
  - The schema: (no)
  - The default values of configurations: (no)
  - The wire protocol: (no)
  - The rest endpoints: (no)
  - The admin cli options: (no)
  - Anything that affects deployment: (no)

### Documentation

Check the box below or label this PR directly.

Need to update docs? 

- [ ] `doc-required` 
(Your PR needs to update docs and you will update later)
  
- [ ] `doc-not-needed` 

  
- [x] `doc` 
Python Doc is updated in __init__.py

- [ ] `doc-complete`
(Docs have been already added)
@Technoboy- Technoboy- changed the title [client][python] getLastMessageIdAsync C binding [client][python] Support getLastMessageIdAsync C binding Aug 11, 2022
@Technoboy- Technoboy- changed the title [client][python] Support getLastMessageIdAsync C binding [client][python] Add getLastMessageIdAsync C binding Aug 11, 2022
Technoboy- pushed a commit to merlimat/pulsar that referenced this pull request Aug 16, 2022
* python cc binding for getLastMessageId

* add python Consumer class method and doc

* fix linter issues based on clang-format

* ubuntu linter fix

* try run unit test in ci

* fix doc comment

* test the test case can be ran

### Motivation

Python function getLastMessageId

It is a C binding for apache#16182 to implement get_last_message_id() in Python client.

### Modifications

Add Python/C binding code for get_last_message_id()

### Verifying this change

It compiles.
- [x] Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.


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

*If `yes` was chosen, please highlight the changes*

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API: (yes)
  - The schema: (no)
  - The default values of configurations: (no)
  - The wire protocol: (no)
  - The rest endpoints: (no)
  - The admin cli options: (no)
  - Anything that affects deployment: (no)

### Documentation

Check the box below or label this PR directly.

Need to update docs? 

- [ ] `doc-required` 
(Your PR needs to update docs and you will update later)
  
- [ ] `doc-not-needed` 

  
- [x] `doc` 
Python Doc is updated in __init__.py

- [ ] `doc-complete`
(Docs have been already added)
@michaeljmarshall michaeljmarshall linked an issue Aug 16, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add getLastMessageId feature to Python Client
7 participants