Skip to content

Conversation

@komalatammal
Copy link
Contributor

@komalatammal komalatammal commented Jun 22, 2022

Motivation

Add getLastMessageId method to C++ Consumer.cc to address the missing part in this PR #15993

Modifications

Expose methods to get last message Id in consumer, the C++ client's Consumer class

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

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

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@github-actions
Copy link

@komalatammal Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Jun 22, 2022
@BewareMyPower BewareMyPower added component/client-c++ type/feature The PR added a new feature or issue requested a new feature labels Jun 22, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jun 23, 2022
@BewareMyPower BewareMyPower merged commit c13d1c7 into apache:master Jun 23, 2022
@eolivelli
Copy link
Contributor

@BewareMyPower we need 2 bindings +1 in order to commit a patch.
we should have waited for a second reviewer

@BewareMyPower
Copy link
Contributor

Okay. Got it.

@BewareMyPower
Copy link
Contributor

@eolivelli But I have a concern that should it be a strict constraint or a blocker? Or how long should we wait if no one has time to review or doesn't want to review? I'm afraid currently there are not enough reviewers for all PRs. For example, #16072 is another PR I've merged with only 1 binding +1. But no one reviewed this PR in 5 days after it's opened, though I've pinged reviewers again.

@eolivelli
Copy link
Contributor

yes, it is a strict constraint for all non-trivial changes.

if you want to sponsor a patch the best way is to send a message to dev@ and ask for a second reviewer.

5 days it not much time in a OSS community, we have to keep in mind that committers are volunteers.
there is no hurry in committing a patch usually and we can always ping the community if we think that a patch is really important or urgent.

@BewareMyPower
Copy link
Contributor

Okay, I'll keep it in mind next time.

@michaeljmarshall
Copy link
Member

yes, it is a strict constraint for all non-trivial changes.

@eolivelli - I did not realize this was a strict requirement. Have we discussed this on the dev mailing list recently? It might be worth re-iterating the requirement. It might also be worth documenting here: https://github.com/apache/pulsar/wiki/Committer-Guide.

michaeljmarshall pushed a commit that referenced this pull request Aug 9, 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- 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- 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)
merlimat pushed a commit to apache/pulsar-client-python that referenced this pull request Sep 29, 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/pulsar#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)
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 type/feature The PR added a new feature or issue requested a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants