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

PIP-64: [REST] Rest API Produce message. #8125

Merged
merged 30 commits into from
Sep 29, 2021

Conversation

MarvinCai
Copy link
Contributor

@MarvinCai MarvinCai commented Sep 24, 2020

Motivation

PIP 64: https://github.com/apache/pulsar/wiki/PIP-64%3A-Introduce-REST-endpoints-for-producing%2C-consuming-and-reading-messages

Tested with Postman

Modifications

Add produce message rest api.

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): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@MarvinCai
Copy link
Contributor Author

@codelipenghui @sijie I'm still refining the implementation and will work on unit test, but can you take a initial look to see if I'm on the right track? Thanks.

@sijie sijie added this to the 2.7.0 milestone Sep 24, 2020
@MarvinCai MarvinCai changed the title [WIP][REST]Initial change to Produce message Rest API. [REST]Initial change to Produce message Rest API. Oct 19, 2020
@MarvinCai MarvinCai changed the title [REST]Initial change to Produce message Rest API. [REST] Rest API Produce message. Oct 19, 2020
@MarvinCai
Copy link
Contributor Author

@codelipenghui Produce logic is working now, I've tested with postman. Can you help take a look on the PR?

@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

@MarvinCai
Copy link
Contributor Author

talked with @sijie and we think the produce message request/response here share many common attributes with WebSocket produce request/response, so instead of creating new class here we should try update the class there and add necessary attributes. This will also help on adding schema support for WebSocket API.
So I'll make the change for updating message request/response first.

@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

move to 2.8.0 first.

@codelipenghui codelipenghui modified the milestones: 2.7.0, 2.8.0 Nov 17, 2020
@MarvinCai
Copy link
Contributor Author

@codelipenghui I've update the url to /topics/[persistent/non-persistent]/tenant/namespace/topic
I think we still need to domain prefix as all other topics related rest endpoint to know if it's a persistent or non-persistent topic, else the code have to make assumption about topics domain which might be different than what user expect

@@ -34,7 +34,7 @@
* 2.The whole range of hash value could be covered by all the consumers.
* 3.Once a consumer is removed, the left consumers could still serve the whole range.
*
* Initializing with a fixed hash range, by default 2 << 5.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an unrelated change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/StickyKeyConsumerSelector.java#L28
Just fixing an inconsistent doc when I saw it, probably doesn't worth a separate for such one line change.

Copy link
Contributor

@315157973 315157973 left a comment

Choose a reason for hiding this comment

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

Overall looks good

@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

@MarvinCai
Copy link
Contributor Author

@eolivelli please help take a look

@codelipenghui codelipenghui merged commit b2678be into apache:master Sep 29, 2021
@eolivelli eolivelli changed the title [REST] Rest API Produce message. PIP-64: [REST] Rest API Produce message. Oct 6, 2021
@codelipenghui codelipenghui added the doc-required Your PR changes impact docs and you will update later. label Nov 1, 2021
@Anonymitaet
Copy link
Member

Discussed with @MarvinCai, he will provide tech inputs for “produce msgs” (we’ll create “REST API” chapter under “Client libraries” this weekend.

@Anonymitaet
Copy link
Member

Doc is added #12918

@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Nov 25, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@billowqiu
Copy link
Contributor

Is there a plan to support consume messages with rest api?@MarvinCai @jiazhai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-complete Your PR changes impact docs and the related docs have been already added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants