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

[feat]: Support client.getPartitionsForTopic (#320) #322

Merged
merged 2 commits into from
May 9, 2023

Conversation

XXXMrG
Copy link
Contributor

@XXXMrG XXXMrG commented Apr 28, 2023

Master Issue: #320

Motivation

Support api: client.getPartitionsForTopic

Modifications

Just bind CPP function: pulsar_client_get_topic_partitions_async to JS.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • will add end to end test later.

Documentation

  • 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)

@XXXMrG
Copy link
Contributor Author

XXXMrG commented Apr 28, 2023

TODO:

  • add end-to-end test case later
  • update doc

@shibd
Copy link
Member

shibd commented May 4, 2023

@XXXMrG Thanks for your contribution. Let's wait for your TODO list to be done.

@shibd shibd marked this pull request as draft May 4, 2023 01:45
@XXXMrG
Copy link
Contributor Author

XXXMrG commented May 8, 2023

Done. Would you mind giving my code a quick review? @shibd

@shibd shibd marked this pull request as ready for review May 8, 2023 02:31
Copy link
Member

@shibd shibd left a comment

Choose a reason for hiding this comment

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

Thanks! Left some small comments.

tests/end_to_end.test.js Outdated Show resolved Hide resolved
tests/end_to_end.test.js Outdated Show resolved Hide resolved
@XXXMrG
Copy link
Contributor Author

XXXMrG commented May 8, 2023

@shibd
I have modified the code according to your comments.
Also, I noticed the compile failed on Windows x86 platforms. I will check it later.

@shibd
Copy link
Member

shibd commented May 8, 2023

@shibd I have modified the code according to your comments. Also, I noticed the compile failed on Windows x86 platforms. I will check it later.

Nice, Let's run ci look at it.

@XXXMrG
Copy link
Contributor Author

XXXMrG commented May 8, 2023

@shibd
I have fixed the compile problem in Windows x86 platforms.

But I found that the Client/getPartitionsForTopic test will fail in the CI environment due to an unexpected extra item that duplicates the topic name in the returned result, it does not happened in my device, this may be related to the Pulsar version in the CI environment, which is 2.10.2, and i use pulsar 2.11.x.
I need some assistance to resolve this issue, thx.

@shibd
Copy link
Member

shibd commented May 9, 2023

2023-05-09T02:41:35,995+0000 [pulsar-web-48-8] WARN  org.eclipse.jetty.server.HttpChannelState - unhandled due to prior sendError
javax.servlet.ServletException: javax.servlet.ServletException: java.lang.NumberFormatException: For input string: ""4""
	at org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:162) ~[org.eclipse.jetty-jetty-server-9.4.48.v20220622.jar:9.4.48.v20220622]
	at org.eclipse.jetty.server.handler.StatisticsHandler.handle(StatisticsHandler.java:181) ~[org.eclipse.jetty-jetty-server-9.4.48.v20220622.jar:9.4.48.v20220622]

Maybe the request body format is invalid. Will cause the partition topic to create failed.

  1. You should assert the HTTP response code.
  2. You should be change request body format:
-          data: '4',
+          data: 4,

@XXXMrG
Copy link
Contributor Author

XXXMrG commented May 9, 2023

@shibd Done. let's run ci check again.

@XXXMrG
Copy link
Contributor Author

XXXMrG commented May 9, 2023

@shibd
It looks like all checks have passed. Can we merge this pull request now?

@shibd shibd merged commit a31b29a into apache:master May 9, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants