Removing yca from CPP client #290
Conversation
|
what's the plan for ABI compat break? force a rebuild to take the new client? |
|
👍 |
|
@merlimat - Changes for #241 are incomplete since we don't have the macro in Auth.cc so if you try to compile the code with -DPULSAR_ENABLE_DEPRECATED_METHOD it will fail since there is no implementation for
@msb-at-yahoo - we are planning to maintain an internal branch with changes for "Auth.h" and "Auth.cc", we will pull changes from OS master and push to the internal branch whenever we build a tag. Since these Auth.* files are rarely changed we will rarely have to do a manual merge. |
|
@jai1 Fine with me.. I was also skeptic of feasibility of source compatibility. Please make sure to check in with @saandrews . |
|
makes sense. longer term we should look at something influenced by SASL so that support for internal auth protocols don't break the ABI.
On Monday, March 13, 2017, 2:12:40 PM PDT, jai1 <notifications@github.com> wrote:
@merlimat - Changes for #241 are incomplete since we don't have the macro in Auth.cc so if you try to compile the code with -DPULSAR_ENABLE_DEPRECATED_METHOD it will fail since there is no implementation for
- AuthData
- static AuthDataPtr Disabled();
- static AuthDataPtr YcaV1(const std::string& ycaAppId);
@msb-at-yahoo - we are planning to maintain an internal branch with changes for "Auth.h" and "Auth.cc", we will pull changed from OS master and push to the internal branch whenever we build a tag.
Since these Auth.* files are rarely changed we will rarely have to do a manual merge.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
It's already possible, the new auth api takes a "plugin" library as a parameter, so you can pass in a new auth implementation. The problem was related to the earlier C++ auth api that was having YCA as an always viable option. |
saandrews
left a comment
saandrews
left a comment
There was a problem hiding this comment.
The hope is that with the plug-in model of Auth, these 2 files would hardly change, hence the local branch would rarely conflict.
|
@merlimat @msb-at-yahoo - I think we already broke the ABI by changing Trying to fix it without having to change Client.cc |
ABI was broken on renaming the c++ namespace from |
|
Please don't merge the branch I will me making more changes to fix the broken ABI so that we can compile by adding -Dpulsar=cms |
jai1
force-pushed
the
yahoo
branch
2 times, most recently
from
4fba118 to
21ad622
Compare
pulsar-client-cpp/CMakeLists.txt
Outdated
There was a problem hiding this comment.
Why do we need this message?
pulsar-client-cpp/lib/CMakeLists.txt
Outdated
jai1
force-pushed
the
yahoo
branch
2 times, most recently
from
643eff2 to
cbbb56f
Compare
|
@msb-at-yahoo @saandrews @merlimat - With this we maintain backward compatibility with Yahoo code (only -Dpulsar=cms and Auth.*) and leave a smaller footprint of depreciated methods. Need a +1 on this please |
| } | ||
| } | ||
|
|
||
| using namespace pulsar; |
There was a problem hiding this comment.
Removed using namespace pulsar;
merlimat
left a comment
merlimat
left a comment
There was a problem hiding this comment.
LGTM, though I can't really verify the binary compatibility
Fix apache#290 This PR supports continuous offset for kop. Since this PR disable the orginal design of mapping between Pulsar `MessageId` and Kafka `offset`. So I just ignore some UnitTest based on the original desing. Maybe we can raise up another issue to track how we deal with these ignored tests.
4 participants
Motivation
Want to reduce yca usage in pulsar
Modifications
Removed code under PULSAR_ENABLE_DEPRECATED_METHOD
Result