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

Added pulsar C++ client #185

Merged
merged 1 commit into from
Feb 7, 2017
Merged

Added pulsar C++ client #185

merged 1 commit into from
Feb 7, 2017

Conversation

saandrews
Copy link
Contributor

Motivation

Added Pulsar C++ Client

Modifications

Added pulsar-client-cpp which contains C++ client implementation, relevant tests & tools

Result

Travis build will also produce libpulsar.so and libpulsar.a and deploy it under releases. The library is built using ubuntu14 since ubuntu16 environment is not yet available in travis. pulsar-client-cpp/README contains instruction to build it in ubuntu16

@rdhabalia rdhabalia added this to the 1.17 milestone Feb 1, 2017
@merlimat merlimat added the type/feature The PR added a new feature or issue requested a new feature label Feb 1, 2017
merlimat
merlimat reviewed Feb 1, 2017
View reviewed changes
.travis.yml Outdated
provider: releases
api_key:
secure: mIACMQlTzepBa9wWbKlME1Ig9/vKLHz63fn2zR2fbYjaAgXCPVe1lbTLbOZb62yB0XL7rUekyKIf0DEFH/cS1XjCb7aDKo4pLh6uiHdIIS8t1qEJJT54vbLwnWJXjo+14QHvaXgDlO/YoxpyOicrKI++b3fScD0zK2I8R6Lmwan/ZQze9uhRO0RKGChsDAszy+98C6JJxQXWQ0YjnUhwP5PtZX3Fm1rxtuCIk2Fl9gQdp9/j9U6vRKtWaO22Q2YaaaPGGoVyTwV6iMSOXDMb6zhjEQ3aiuJMJHUJRcGEU4fV7hkiUukWdo5+5C/mASNiJDYefG86KfCktMniPMzyAPXNc6hUzbOZuLNI1/f1QqBwzTJbH7NIUjz5f0hjNsHuYvkL8TcxE9pDA0Qkr8OIWR8M3+H7WKuiSTaVSeCobGBE8g6ymanlRvOQZblFpgw91B/KmZucsin0+rV5tVRlqTBYHL5f6fXEyhKdGYRiHaNR29mBBJsZng2tR6wVjPGqyEfdwFVOs44d2Rkt885VjZthap/Yw+SJKOvbJv1zaRglmbvbl629LvYOgT6ptYPDJyu/J/kzPrWnzvyTf72M6bR991Kx8gEkT4WRwCRBAuhg8i2bmIcsjbXtLcB0YRHgrBueJD0SuLREtcxJYvkgMI1UZon5UrCTkJDc0oFLO28=
file_glob: true
file: "all/target/pulsar-*.tar.gz"
file: "pulsar-client-cpp/lib/libpulsar.*"
Copy link
Contributor

Choose a reason for hiding this comment

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

These are the files that are uploaded to Github release page.

If we want to release binaries for C++ client, we should pack them into a deb/rpm package related to a specific ubuntu/rhel release.

I'd say, for now to skip this part and just release the sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets leave it there if someone would like to consume until we generate package.

Copy link
Contributor

Choose a reason for hiding this comment

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

uhm, but that so file is depending on some specific version of dependencies libs, libstdc++, etc.. It would be pretty difficult to use as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats true. We were also debating the same. I'll remove it.

.travis.yml Outdated
# Reconstruct the gpg keys to sign the artifacts
before_deploy:
- echo $GPG_SECRET_KEYS | base64 --decode | $GPG_EXECUTABLE --import --batch || true
- echo $GPG_OWNERTRUST | base64 --decode | $GPG_EXECUTABLE --import-ownertrust --batch || true

install:
- sudo apt-get install -y cmake libssl-dev libcurl4-openssl-dev liblog4cxx10-dev libprotobuf-dev libboost1.55-all-dev libgtest-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 parts could be enclosed in a bash script, to get more readability.

pulsar-client-cpp/CMakeLists.txt
project (pulsar-cpp)

set(Boost_NO_BOOST_CMAKE ON)

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that would be good to have is to get Pulsar version from the top level pom and add that as a compiler macro definition. This way the client will be able to report its own version to the broker (same as Java client).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it in lib/CMakeLists.txt

pulsar-client-cpp/tests/standalone.conf
@@ -0,0 +1,254 @@
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using a special config file, we could change the tests to point to port 6650

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to leave the standalone port as it is since it might be running real workload.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, no problem

@merlimat merlimat added the c++ label Feb 1, 2017
merlimat
merlimat reviewed Feb 1, 2017
View reviewed changes
.travis.yml Outdated
# Reconstruct the gpg keys to sign the artifacts
before_deploy:
- echo $GPG_SECRET_KEYS | base64 --decode | $GPG_EXECUTABLE --import --batch || true
- echo $GPG_OWNERTRUST | base64 --decode | $GPG_EXECUTABLE --import-ownertrust --batch || true

install:
- sudo apt-get install -y cmake libssl-dev libcurl4-openssl-dev liblog4cxx10-dev libprotobuf-dev libboost1.55-all-dev libgtest-dev
- pushd $HOME/ && wget https://github.com/google/protobuf/releases/download/v2.6.1/protobuf-2.6.1.tar.gz && popd
Copy link
Contributor

Choose a reason for hiding this comment

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

It might also be good to cache both the tgz and the compiled stuff in travis, see line 6.

@saandrews saandrews force-pushed the master branch 5 times, most recently from 48c3f4e to 18d5d61 Compare February 1, 2017 23:50
merlimat
merlimat reviewed Feb 1, 2017
View reviewed changes
pulsar-client-cpp/lib/CMakeLists.txt Outdated
add_library(pulsarStatic STATIC ${PULSAR_SOURCES})
add_library(pulsarShared SHARED ${PULSAR_SOURCES})

set_target_properties(pulsarStatic PROPERTIES OUTPUT_NAME pulsar VERSION $ENV{LIB_VERSION})
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, this will set the version on the compiled so library. I was thinking more about having a macro defined like -DPULSAR_VERSION="1.16" in gcc command line. Then using a version.h header that captures that and finally send the version to the broker in the connect command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, Just addressed it.

@saandrews saandrews force-pushed the master branch 4 times, most recently from 70898d5 to 862d019 Compare February 2, 2017 04:09
merlimat
merlimat reviewed Feb 2, 2017
View reviewed changes
pulsar-client-cpp/lib/Version.h
#define LIB_VERSION_H_

#ifndef _PULSAR_VERSION_
#define _PULSAR_VERSION_ "1.17"
Copy link
Contributor

Choose a reason for hiding this comment

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

I was meaning to use the version from the same variable, so that we don't need to remember to change it each time. 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is done by build.sh which reads from the pom file and defines it during compilation. The define here is to just have a default value. Maybe we can change it to undefined or remove the default. This way, compilation will fail if not defined and force user to define it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. Then maybe grepping for the version could be done directly inside the CMakelist.txt so that it works even when not using build.sh script (not sure exactly how that works though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed this in lib/CMakeLists.txt. Now, cmake itself will take care of it.

@merlimat
Copy link
Contributor

merlimat commented Feb 6, 2017

@saandrews @rdhabalia Added reading version from inside cmake definition here:
merlimat@6aa7c99

@merlimat
Copy link
Contributor

merlimat commented Feb 7, 2017

Looks good to me. One final minor aks: can you rename build.sh into travis-build.sh?

@saandrews
Copy link
Contributor Author

Updated. I will merge once CI is complete?

merlimat
merlimat approved these changes Feb 7, 2017
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.

👍

@saandrews saandrews merged commit 77f1ce8 into apache:master Feb 7, 2017
sijie pushed a commit to sijie/pulsar that referenced this pull request Mar 4, 2018
@srkukarni @sijie
Since server is already module name, rename the variable (apache#185)
a954d96
hangc0276 pushed a commit to hangc0276/pulsar that referenced this pull request May 26, 2021
@josephglanville
Create cluster if it doesn't exist during metadata initialisation (ap…
5ee6bf0 
…ache#445)

This case is usually encountered when attempting to use KoP with standalone mode. When new behavior was introduced in apache#168 standalone mode was broken due to sample namespace setup etc not taking place until after the broker is initialized (which in turn initializes protocol handlers).

Fixes apache#185
@xiaotongwang1 xiaotongwang1 mentioned this pull request Aug 4, 2021
Closed
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

Assignees

@saandrews saandrews

Labels
area/client type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Milestone
1.17
Development

Successfully merging this pull request may close these issues.
3 participants
@saandrews @merlimat @rdhabalia