HTTP lookups for c++ client lib #317

jai1 · 2017-03-27T17:11:59Z

Motivation

Want to reintroduce HTTP Lookup in order to maintain seamless production rollout

Modifications

Couldn't use URDL hence created a HTTPWrapper over asio

Result

HTTPLookup possible.

TODO - Remaining

a. Remove mentions of YCA from the code

msb-at-yahoo

looks decent, but it's somewhat unfortunate that we'd need to implement an HTTP-subset client library for this.

what was the issue you ran into with using curl? integrating with the asio event loop?

msb-at-yahoo · 2017-03-29T17:47:27Z

pulsar-client-cpp/lib/ClientImpl.cc

          producerIdGenerator_(0),
          consumerIdGenerator_(0),
          requestIdGenerator_(0) {
        LogUtils::init(clientConfiguration.getLogConfFilePath());
+        if ( !strncmp(serviceUrl_.c_str(), "http", 4)) {


if (serviceUrl.compare(0, 4, "http") == 0)

msb-at-yahoo · 2017-03-29T17:48:19Z

pulsar-client-cpp/lib/ClientImpl.cc

          producerIdGenerator_(0),
          consumerIdGenerator_(0),
          requestIdGenerator_(0) {
        LogUtils::init(clientConfiguration.getLogConfFilePath());
+        if ( !strncmp(serviceUrl_.c_str(), "http", 4)) {
+            LOG_DEBUG("Using HTTP Lookup");
+            lookupServicePtr_ = boost::make_shared<HTTPLookupService>(boost::ref(serviceUrl_),


boost::cref

msb-at-yahoo · 2017-03-29T17:51:18Z

pulsar-client-cpp/lib/HTTPLookupService.cc

+    const static std::string V2_PATH = "/lookup/v2/destination/";
+    const static std::string PARTITION_PATH = "/admin/persistent/";
+    const static int MAX_HTTP_REDIRECTS = 20;
+    const static char SEPARATOR = '/';


i think the use of a constant for '/' just obfuscates the code below.

msb-at-yahoo · 2017-03-29T18:01:03Z

pulsar-client-cpp/lib/HTTPWrapper.cc

+            os << *iter << "\r\n";
+            iter++;
+        }
+        os << request.content << "\r\n";


you need "\r\n" before the request body and you shouldn't add a "\r\n" after the body, so:
os << "\r\n" << request.content;

msb-at-yahoo · 2017-03-29T18:02:46Z

pulsar-client-cpp/lib/HTTPWrapper.cc

+            : executorServiceProviderPtr_(executorServiceProviderPtr){
+    }
+
+    std::string HTTPWrapper::getHTTPMethodName(const Request::Method& method) {


i think it'd be best to avoid these temporaries.

const std::string& ... switch (method) { case Request::GET: { static const std::string s("GET"); return s; }

Not making this change since if we don't copy the returned result the underlying object is deleted

#include
using namespace std;
const int& get1() {
int i = 1;
return i;
}

int main() {
int a = get1();
// int &b = get1();// compilation error since we drop the const
const int &c = get1();
cout<<a<<endl;
cout<<c<<endl;
}

./a.out
1
32767

msb-at-yahoo · 2017-03-29T18:58:43Z

pulsar-client-cpp/lib/HTTPLookupService.cc

+        LOG_DEBUG("HTTPLookupService::callback response = " << response);
+        if (response.retCode != HTTPWrapper::Response::Success) {
+            if (response.statusCode == 401) {
+                // 401 means unauthorized


401 means unauthenticated, 403 is unauthorized.

msb-at-yahoo · 2017-03-29T18:59:11Z

pulsar-client-cpp/lib/HTTPLookupService.cc

+            const std::vector<std::string>& responseHeaders = response.headers;
+            std::vector<std::string>::const_iterator iter = responseHeaders.begin();
+            while (iter != responseHeaders.end()) {
+                if (!strncmp((*iter).c_str(), "Location", 8)) {


another case where we should prefer std::string::compare to strncmp

msb-at-yahoo · 2017-03-29T19:10:40Z

pulsar-client-cpp/lib/HTTPWrapper.cc

+            // eof occurs but responseStreamPtr_ not necessarily emptys
+            LOG_DEBUG("EOF occured");
+            std::istream inputStream(responseStreamPtr_.get());
+            inputStream >> response_.content;


is the intent to read all of the stream content and store into response_.content? this only reads the first whitespace delimited string. try this:

std::ostringstream os; os << inputStream.rdbuf(); response_.content = os.str();

msb-at-yahoo · 2017-03-29T19:10:56Z

pulsar-client-cpp/lib/HTTPWrapper.cc

+            callback_(shared_from_this());
+        }
+    }
+}


missing newline

msb-at-yahoo · 2017-03-29T19:13:16Z

pulsar-client-cpp/lib/HTTPWrapper.cc

+    static const HTTPWrapper::Response EMPTY_RESPONSE = HTTPWrapper::Response();
+
+    static void removeCarriage(std::string& str) {
+        str.erase( std::remove(str.begin(), str.end(), '\r'), str.end() );


this removes all '\r' occurrences in the string, but in all of the places you use it, there's only one at the end of the line, right?

…cpp version

jai1 · 2017-04-04T20:22:54Z

pulsar-client-cpp/lib/HTTPLookupService.cc

+            return LookupDataResultPtr();
+        }
+        const std::string defaultNotFoundString = "Url Not found";
+        const std::string brokerUrl = root.get("brokerUrl", defaultNotFoundString).asString();


Used get instead of find since the json_cpp version in yahoo env doesn't support the find API.

merlimat

Looks good overall, couple of minor issues

merlimat · 2017-04-05T00:54:38Z

pulsar-client-cpp/CMakeLists.txt

@@ -32,6 +32,11 @@ find_library(LOG4CXX_LIBRARY_PATH log4cxx)
 find_library(CURL_LIBRARY_PATH curl)
 find_path(LOG4CXX_INCLUDE_PATH log4cxx/logger.h)
 find_path(GTEST_INCLUDE_PATH gtest/gtest.h)
+find_library(LIB_JSON jsoncpp)


Can you also update README with the dependency list?

merlimat · 2017-04-05T00:56:40Z

pulsar-client-cpp/lib/HTTPLookupService.cc

+        }
+
+        // Once per application - https://curl.haxx.se/mail/lib-2015-11/0052.html
+        curl_global_init(CURL_GLOBAL_ALL);


If I start 2 pulsar clients, there would be a problem. We need to have some static flag and mutex to ensure this gets executed once.

merlimat · 2017-04-05T01:01:33Z

pulsar-client-cpp/lib/Url.cc

+    return path_;
+}
+
+const std::string& Url::file() const {


What is file in a URL?

Shouldn't that be part of the path?

jai1 · 2017-04-05T18:37:28Z

@merlimat - I have handled your CR comments - can I please get a +1
Also for the URL part:-
The legacy code Url.cc:42 regex was designed for handling URLs of type http://test.example.com/dir/subdir/file.html?arg1=var1,arg2=var2

Where path is "/dir/subdir/" and file is "file.html"

merlimat · 2017-04-05T20:01:07Z

pulsar-client-cpp/lib/HTTPLookupService.cc

+            adminUrl_ = lookupUrl;
+        }
+        // Once per application - https://curl.haxx.se/mail/lib-2015-11/0052.html
+        Lock lock(curlGlobalMutex_);


I don't think this would work either. It just protects 2 threads from calling curl_global_init() at the same time, but not at different times. Also we need to keep track of how many Pulsar instances are created, since we can only call the curl_global_cleanup() once the last instance is destroyed.

I think the easiest option would be to encapsulate the curl_global_init / cleanup within a c++ object managed by a static shared_ptr or something like that.

you don't need a shared pointer:

static void globalInit() { struct Init { Init() { curl_global_init(XXX); } ~Init() { curl_global_cleanup(); } }; // required to be thread-safe by C++11, true for GCC >= 3.4. // FIXME: someone add a #error here for old versions of MSVC static Init init; }

I guess this will work

class HTTPLookupService : public LookupService, public boost::enable_shared_from_this<HTTPLookupService> { class CurlInitializer { public: CurlInitializer() { // Once per application - https://curl.haxx.se/mail/lib-2015-11/0052.html curl_global_init (CURL_GLOBAL_ALL); } ~CurlInitializer() { curl_global_cleanup(); } }; static CurlInitializer curlInitializer;

merlimat

👍

saandrews · 2017-04-05T22:13:29Z

pulsar-client-cpp/lib/ExecutorService.cc

+ReadStreamPtr ExecutorService::createReadStream() {
+    return boost::make_shared<boost::asio::streambuf>();
+}
+


If this is unused, we should remove it.

* Export client metrics to Prometheus * Added missing messagesReceived.Inc()

Fixes apache#315 The original `listeners` config's semantics is wrong that it mixed the `listeners` and `advertised.listeners` semantics of Kafka. So this PR adds a `kafkaAdvertisedListeners` as the listeners exposed to client, and only use `listeners` as the bind address. To avoid conflict with other protocol handlers, mark `listeners` as deprecated and use `kafkaListeners` instead. For convenience, this PR adds an `EndPoint` class to do the listener url parse work, which can be applied to both `kafkaListeners` and `kafkaAdvertisedListeners`. It also handles `SASL_XXX` protocols which were not handled before. And the related tests are added. The existed `KafkaApisTest#testBrokerHandleTopicMetadataRequest` could verify the `kafkaAdvertisedListeners` because the tests' `kafkaAdvertisedListeners` is `127.0.0.1:<port>` while `kafkaListeners` is `localhost:<port>`.

Jai ASher added 9 commits March 22, 2017 22:35

Blocking libcurl

f614e3c

SSwitched to asio

2e8eed9

Working

ad97561

Working so far

c141f8b

Not working

cb716b3

Base working now

e749680

Redirect left

5a4183c

Redirection remains

14e0b89

Added Removed mentions of my name

dc5b2a6

jai1 added c++ area/client labels Mar 27, 2017

jai1 added this to the 1.17 milestone Mar 27, 2017

jai1 self-assigned this Mar 27, 2017

jai1 requested review from rdhabalia, msb-at-yahoo and saandrews March 27, 2017 17:11

Jai ASher added 4 commits March 27, 2017 19:09

Url extension

1943566

Merge branch 'master' of https://github.com/yahoo/pulsar into httplookup

980e211

redirects handled

a6db72d

Corrections in redirects

846ea50

jai1 closed this Mar 28, 2017

Jai ASher added 6 commits March 27, 2017 22:23

Fixed socket resetting logic

26a9aa3

Handled carriage

9c82572

added more error handling

1e59e49

added more error handling

542725f

added more error handling

7a3c76a

added more error handling

e60aaf2

jai1 reopened this Mar 28, 2017

jai1 requested a review from merlimat March 28, 2017 16:51

msb-at-yahoo reviewed Mar 29, 2017

View reviewed changes

Jai ASher added 10 commits April 3, 2017 00:05

Removed HTTPWrapper files

12dc686

Removed mentions of my name

b895c65

Compilation errors resolved

8926955

Formatting issues

b8684aa

Fixed JSON parsing logic since find API is not present in linux JSON …

1085bd8

…cpp version

CR Comments and corrected header logic

cabffde

Corrected Auth part

6c0b24b

Corrected Auth part

5376cd5

changed this to shared_from_this()

455e7f1

Switching on CURLOPT_NOSIGNAL

e94fe7e

jai1 commented Apr 4, 2017

View reviewed changes

jai1 mentioned this pull request Apr 4, 2017

Create HTTP Lookup for CPP Client #307

Closed

merlimat reviewed Apr 5, 2017

View reviewed changes

merlimat changed the title ~~Httplookup~~ HTTP lookups for c++ client lib Apr 5, 2017

Jai ASher added 3 commits April 5, 2017 10:31

Handled Matteo's Code Review Comments

40a4d33

Merge branch 'master' of https://github.com/yahoo/pulsar into httplookup

631334e

Fixed compilation error

764efff

merlimat reviewed Apr 5, 2017

View reviewed changes

Jai ASher added 2 commits April 5, 2017 14:10

Made curl initialization happen once

6c7e011

Rearranged the class

18fe553

merlimat approved these changes Apr 5, 2017

View reviewed changes

saandrews reviewed Apr 5, 2017

View reviewed changes

Addresses Andrew's CR comments

2d0d9fc

saandrews approved these changes Apr 5, 2017

View reviewed changes

jai1 merged commit bea8ff2 into apache:master Apr 5, 2017

jai1 deleted the httplookup branch April 5, 2017 23:14

hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020

Export client metrics to Prometheus (apache#317)

220dcb4

* Export client metrics to Prometheus * Added missing messagesReceived.Inc()

xiaotongwang1 mentioned this pull request Aug 4, 2021

Pulsar 2.7.0+ KOP 2.7.2.x getPartitionedTopicMetadata timeout #11532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP lookups for c++ client lib #317

HTTP lookups for c++ client lib #317

jai1 commented Mar 27, 2017 •

edited

Loading

msb-at-yahoo left a comment

msb-at-yahoo Mar 29, 2017

msb-at-yahoo Mar 29, 2017

msb-at-yahoo Mar 29, 2017

msb-at-yahoo Mar 29, 2017

msb-at-yahoo Mar 29, 2017

jai1 Mar 30, 2017

msb-at-yahoo Mar 29, 2017

msb-at-yahoo Mar 29, 2017

msb-at-yahoo Mar 29, 2017

msb-at-yahoo Mar 29, 2017

jai1 Mar 30, 2017

msb-at-yahoo Mar 29, 2017

jai1 Apr 4, 2017

merlimat left a comment

merlimat Apr 5, 2017

jai1 Apr 5, 2017

merlimat Apr 5, 2017

merlimat Apr 5, 2017

merlimat Apr 5, 2017

jai1 commented Apr 5, 2017

merlimat Apr 5, 2017

msb-at-yahoo Apr 5, 2017

jai1 Apr 5, 2017

merlimat left a comment

saandrews Apr 5, 2017

HTTP lookups for c++ client lib #317

HTTP lookups for c++ client lib #317

Conversation

jai1 commented Mar 27, 2017 • edited Loading

Motivation

Modifications

Result

TODO - Remaining

msb-at-yahoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jai1 commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jai1 commented Mar 27, 2017 •

edited

Loading