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.

Sign up for GitHub

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

Jump to bottom

Issue #3332: fix httplookup issue for get ns/topics in cpp #3407

Merged
merged 2 commits into from Jan 24, 2019
Merged

Issue #3332: fix httplookup issue for get ns/topics in cpp #3407

merged 2 commits into from Jan 24, 2019

Conversation

jiazhai
Copy link
Member

@jiazhai jiazhai commented Jan 23, 2019

Fixes #3332

Motivation

In http request to get ns/topics, it will return an array of topics,

$ curl -L 'http://127.0.0.1:8080/admin/v2/namespaces/foo/bar/topics'
["persistent://foo/bar/test-aa","persistent://foo/bar/test-ab"]

But in cpp method HTTPLookupService::parseNamespaceTopicsData, it is expecting a json of item
like: {"topics": ["topic1", "topic2"] }. This may be caused by the response format changes.

Modifications

  • change cpp method HTTPLookupService::parseNamespaceTopicsData to following current http response data format.
  • changed cpp test testPatternMultiTopicsConsumerPubSub to use http adminUrl, to protect this part of cpp code.

Verifying this change

cpp unit tests passed, especially for test testPatternMultiTopicsConsumerPubSub

@jiazhai jiazhai added type/bug The PR fixed a bug or issue reported a bug component/c++ labels Jan 23, 2019
@jiazhai jiazhai self-assigned this Jan 23, 2019
@jiazhai
Copy link
Member Author

jiazhai commented Jan 23, 2019 •
edited

rerun cpp tests

failed for:

2019-01-23\T\10:28:56.730 [INFO] Copying files to /home/jenkins/jenkins-slave/workspace/pulsar_precommit_cpp/pulsar-sql/presto-pulsar-plugin/target/pulsar-presto-connector
2019-01-23\T\10:28:56.854 [WARNING] Assembly file: /home/jenkins/jenkins-slave/workspace/pulsar_precommit_cpp/pulsar-sql/presto-pulsar-plugin/target/pulsar-presto-connector is not a regular file (it may be a directory). It cannot be attached to the project build for installation or deployment.
2019-01-23\T\10:28:56.854 [INFO] 
2019-01-23\T\10:28:56.854 [INFO] --- maven-install-plugin:2.5.2:install (default-install) @ pulsar-presto-connector ---
2019-01-23\T\10:28:56.856 [INFO] Installing /home/jenkins/jenkins-slave/workspace/pulsar_precommit_cpp/pulsar-sql/presto-pulsar-plugin/target/pulsar-presto-connector.jar to /home/jenkins/.m2/repository/org/apache/pulsar/pulsar-presto-connector/2.3.0-SNAPSHOT/pulsar-presto-connector-2.3.0-SNAPSHOT.jar
2019-01-23\T\10:28:56.857 [INFO] Installing /home/jenkins/jenkins-slave/workspace/pulsar_precommit_cpp/pulsar-sql/presto-pulsar-plugin/pom.xml to /home/jenkins/.m2/repository/org/apache/pulsar/pulsar-presto-connector/2.3.0-SNAPSHOT/pulsar-presto-connector-2.3.0-SNAPSHOT.pom
2019-01-23\T\10:28:56.859 [INFO] Installing /home/jenkins/jenkins-slave/workspace/pulsar_precommit_cpp/pulsar-sql/presto-pulsar-plugin/target/pulsar-presto-connector.tar.gz to /home/jenkins/.m2/repository/org/apache/pulsar/pulsar-presto-connector/2.3.0-SNAPSHOT/pulsar-presto-connector-2.3.0-SNAPSHOT.tar.gz

@jiazhai
Copy link
Member Author

jiazhai commented Jan 23, 2019

rerun cpp tests

merlimat
merlimat reviewed Jan 23, 2019
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of comments

pulsar-client-cpp/lib/HTTPLookupService.cc Outdated
@@ -310,18 +310,21 @@ LookupDataResultPtr HTTPLookupService::parseLookupData(const std::string &json)
NamespaceTopicsPtr HTTPLookupService::parseNamespaceTopicsData(const std::string &json) {
Json::Value root;
Json::Reader reader;
LOG_INFO("GetNamespaceTopics json = " << json);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be at debug level

pulsar-client-cpp/tests/BasicEndToEndTest.cc Outdated
@@ -1756,7 +1756,7 @@ TEST(BasicEndToEndTest, testPatternTopicsConsumerInvalid) {
// verify PatternMultiTopicsConsumer subscribed matched topics,
// and only receive messages from matched topics.
TEST(BasicEndToEndTest, testPatternMultiTopicsConsumerPubSub) {
Client client(lookupUrl);
Client client(adminUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should have the test fetching the lists of topic using both HTTP and protobuf.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, will add a new test.

@merlimat merlimat added this to the 2.3.0 milestone Jan 23, 2019
sijie
sijie approved these changes Jan 24, 2019
View reviewed changes
pulsar-client-cpp/lib/HTTPLookupService.cc Outdated
@@ -310,18 +310,21 @@ LookupDataResultPtr HTTPLookupService::parseLookupData(const std::string &json)
NamespaceTopicsPtr HTTPLookupService::parseNamespaceTopicsData(const std::string &json) {
Json::Value root;
Json::Reader reader;
LOG_INFO("GetNamespaceTopics json = " << json);
Copy link
Member

Choose a reason for hiding this comment

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

nit: debug

@jiazhai
Copy link
Member Author

jiazhai commented Jan 24, 2019

rerun integration tests

2 similar comments
@jiazhai
Copy link
Member Author

jiazhai commented Jan 24, 2019

rerun integration tests

@jiazhai
Copy link
Member Author

jiazhai commented Jan 24, 2019

rerun integration tests

merlimat
merlimat approved these changes Jan 24, 2019
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@merlimat merlimat merged commit 0ec897f into apache:master Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@sijie sijie sijie approved these changes

@srkukarni srkukarni Awaiting requested review from srkukarni

Assignees

@jiazhai jiazhai

Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jiazhai @merlimat @sijie