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

[ROCKETMQ-268]add rocketmq-cpp code #27

Closed
wants to merge 35 commits into from
Closed

[ROCKETMQ-268]add rocketmq-cpp code #27

wants to merge 35 commits into from

Conversation

vincentWangKB
Copy link
Contributor

add rocketmq-cpp code

@lizhanhui
Copy link
Contributor

Trying to create a CMakeLists.txt on top of this PR, but it looks there is something missing:

MetaqJson::Value json

I grepped MetaqJson in the code, but no definition of this struct if found. Please double check the code is compilable.

@lizhanhui
Copy link
Contributor

A second issue about alog, there are only header files and binary static lib file, where is source code for the static lib files?

@lizhanhui
Copy link
Contributor

About the disruptor module in src/thread, copyright declarations of this module files are different. Is the source code copied from somewhere? If so, is its license compatible with Apache Software Foundation?

@lizhanhui
Copy link
Contributor

Many useless files are included, for example in project folder. Can we remove them?

@lizhanhui
Copy link
Contributor

As it's now part of the Apache RocketMQ project, IMO, rocketmq namespace should be used instead of metaq.

@vincentWangKB
Copy link
Contributor Author

For build and install, cmake and Makefile are all supported on linux platform now. Thanks

@vincentWangKB vincentWangKB changed the title add rocketmq-cpp code [ROCKETMQ-268]add rocketmq-cpp code Aug 15, 2017
@vincentWangKB
Copy link
Contributor Author

@lizhanhui all build issue had been fixed, please pull latest code, thanks

@lizhanhui
Copy link
Contributor

Yes, I see your update...Thanks for you contribution. CMake, IMO, is very good to have!

One more thing, I think we can assume dependent libraries are all installed in the standard default way, thus, we can use find_package features and developers do not have to modify CMake build file. If required libraries are missing, we just abort in the cmake phase.

@vincentWangKB
Copy link
Contributor Author

@lizhanhui alog lib had been instead by boost::log, thanks your advisement

@vesense
Copy link
Member

vesense commented Aug 16, 2017

I suggest that we'd better squash the commits before merging a PR.

@lizhanhui
Copy link
Contributor

@vesense Yes, we will do this before actually merging

@lizhanhui
Copy link
Contributor

@vincentWangKB Another issue with the code, currently all classes sit in the namespace of metaq, we should rename it to rocketmq.

@lizhanhui
Copy link
Contributor

@vincentWangKB Any progress? It's been a while since last update to this PR.

@lizhanhui
Copy link
Contributor

Maybe, we can merge it and create JIRA tickets to polish the code. Any idea?

@vincentWangKB
Copy link
Contributor Author

@lizhanhui namespace had been changed to rocketmq, thanks

@lizhanhui
Copy link
Contributor

Though there are still quite a few unused files, I am fine to merge this PR first and fix issues later on.

@lizhanhui
Copy link
Contributor

This patch looks good enough to be merged. Thanks @vincentWangKB

asfgit pushed a commit that referenced this pull request Sep 4, 2017
remove unused boost file

update README.md

update readme.md

update readme.md

update readme

add gitkeep to bin

update readme.md

update apache license info

remove static dependency

update readme.md

add jsoncpp and libevent install discription

udpate makefile

update jsoncpp version

unify dependency

update readme

support cmake

instead alog by boost::log

For cmake, remove alog

remove alog comment

[add rocketmq-cpp code #27] add cmake to readme

use rocketmq namespace, update json version to 0.10.6

update cmakelist

update json version to 0.10.6

remove makefile

add default search path for jsoncpp and libevent

Update README.md

fix could not find jsoncpp issue

support build on new g++ version

Remove built .o files
@lizhanhui
Copy link
Contributor

This Pull Request has been merged and it's safe to close this PR. @vincentWangKB

@vongosling
Copy link
Member

It is a good practice for apache way :-)

@XadillaX
Copy link

Consider removing boost? This dependency is too large for a SDK not a complete project.

@vincentWangKB
Copy link
Contributor Author

close pull request, as rocketmq cpp code had been merged

huanwei added a commit to huanwei/rocketmq-externals that referenced this pull request Apr 8, 2019
Signed-off-by: huanwei <huan@harmonycloud.cn>
@huanwei huanwei mentioned this pull request Apr 8, 2019
6 tasks
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

5 participants