apache
[C++] pulsar-client-cpp stability enhancements and code cleanup
Howdy! My company is investing heavily into event driven workflows, and we're utilizing Pulsar to accomplish this end. We're primarily a C++ shop, and I'm a Principal Engineer and in charge of the integration of Pulsar into our system, and experienced in C++. I've made a few bug fixes to the C++ client already, and in doing so I've noticed that it could use a bit of love to bring greater consistency to the design and fix up some low-hanging fruit. We've integrated Pulsar into our own build system, and it required a lot of changes to pass our build (which uses many more warnings and runs tests with ASAN). We've found that enforcing a more strict set of warnings helps to keep a c++ codebase healthy and stable, and I'd like to upstream these changes for the benefit of everyone, and to make continued maintenance easier.
Would anyone be opposed to me undertaking some general cleanup of the C++ client?
My proposed TODO list:
- Enable many more warnings (
-Wall,-Wvla,-Wformat-security, etc) - Enforce warnings as errors to prevent changes being submitted with warnings
- Fix all the subsequent warnings that will be generated
- Fix compatibility with boost 1.41 (the documentation claims it should be compatible, but it's not, and that happens to be the version we're stuck on).
- Use consistent format for
#includes of local files (some have a/libprefix, others don't) - Run the tests via ASAN or Valgrind, if available (not sure how easy this will be yet...)
- Update syntax to take advantage of more C++11 features (like
std::make_shared, move semantics, etc)
I'll obviously split this stuff up into a series of smaller changes. I'm hopeful that by doing this I'll gain more experience working with Pulsar and could potentially contribute more significant improvements in the future, i.e. performance enhancements and helping to keep feature parity between the C++ and Java clients.
Thanks!
LGTM, here're my comments.
- Enable many more warnings (-Wall, -Wvla, -Wformat-security, etc)
- Enforce warnings as errors to prevent changes being submitted with warnings
- Fix all the subsequent warnings that will be generated
Great. Currently there're a lot of compile warnings and it would be better to eliminate all these warnings.
Fix compatibility with boost 1.41 (the documentation claims it should be compatible, but it's not, and that happens to be the version we're stuck on).
It would be helpful for Boost incompatibilities. But I just have one concern. Since some methods of newer Boost might not be available for older Boost, it's easy to break the Boost incompatibility even after you fixed the incompatibility. But I don't think it's good to downgrade the Boost dependency of image for CI.
Use consistent format for #includes of local files (some have a /lib prefix, others don't)
I've just done this work several days ago but forgot to submit a PR. I can open a PR later. The original purpose is that currently Pulsar C++ client doesn't obey PImpl idiom well and some headers have many included headers, which adds a lot of unnecessary compile time if some files are changed.
Run the tests via ASAN or Valgrind, if available (not sure how easy this will be yet...)
Looks good. But I'm also not sure how easy this will be :(
Update syntax to take advantage of more C++11 features (like std::make_shared, move semantics, etc)
Great. There're much old legacy code currently, like explicit iterator in an iteration, a non-atomic variable with a mutex for thread safety, etc.
Thanks for the feedback guys. I'll proceed with the changes as time allows, starting with the improved warnings.
With respect to the boost compatibility stuff: you're quite right that it's tricky, and I understand your desire to drop support for older versions (especially those from the pre-C++11 era). I'm have no intention to change the preferred boost version in the CI suite.
My company's codebase is a bit of an aberration - it's over 20 years old, and is a >3M line monorepo, so things like upgrading boost is a major undertaking, and 3rd-party library integration is difficult. I certainly don't expect to make this thing out-of-the-box compatible with that sort of setup, but rather I simply want to reduce the friction where it's easy. To make this project compile for our code base under boost 1.41 there were a few cases where I had to go to such lengths as including "patch" headers from newer boost versions. Obviously there's nothing we can do here about completely missing functionality like that.
I'll limit any boost related changes to only simple things that I can cleanly fix by #ifdefing code blocks to specific versions. There's a least a few places where doing that can extend boost support back a few versions, which I think would be generally helpful without disturbing anything else.
