Resolve compatibility with previous version of C++ client #241
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saandrews
reviewed
Feb 24, 2017
Contributor
|
I don't think we can or should maintain compatibility with the internal version. One example is that the C++ namespace was already changed ( |
Contributor
|
Matteo,
It's easier to address the namespace difference just by adding
-Dpulsar=cms during compilation. Even dev packages can be generated by
replacing pulsar to cms. This way, we don't have to maintain 2 versions of
headers and made duplicate changes.
Andrews.
…On Fri, Feb 24, 2017 at 2:22 PM, Matteo Merli ***@***.***> wrote:
I don't think we can or should maintain compatibility with the internal
version. One example is that the C++ namespace was already changed (cms::
to pulsar::) , so the source-level compatibility is already gone.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#241 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ATk4LKzjd5LIum17NMfElSEM9nwRML77ks5rf1gigaJpZM4MKwkl>
.
|
yush1ga
force-pushed
the
resolve_compatibility
branch
from
580e21a to
f5531ed
Compare
Contributor
|
👍 @merlimat your thoughts? I think this is better than maintaining older version of Auth.h and Auth.cc. |
Contributor
|
@saandrews I still don't see how source level compatibility can be maintained. Eg: how can you enable YCA? yca auth implementation is not included in this repo |
merlimat
reviewed
Feb 28, 2017
Contributor
There was a problem hiding this comment.
Can we mark the compatibility method as deprecated? This way we can remove them once all the users updated the code.
Contributor
Author
There was a problem hiding this comment.
Do you mean like following?
#ifdef PULSAR_ENABLE_DEPRECATED_METHOD
enum AuthType {
AuthNone,
AuthYcaV1
};
#endif
Contributor
There was a problem hiding this comment.
This good too. In addition, can you also update the comment to say this is deprecated?
Contributor
There was a problem hiding this comment.
I was thinking about a comment in the code. But the macro looks good
merlimat
reviewed
Feb 28, 2017
Contributor
There was a problem hiding this comment.
YCA macro logic should be reversed, like PULSAR_ENABLE_YCA with default false.
Contributor
|
@saandrews @yush1ga Looks good to me. Change a couple of minor things. |
yush1ga
force-pushed
the
resolve_compatibility
branch
from
f5531ed to
b74fe7d
Compare
yush1ga
force-pushed
the
resolve_compatibility
branch
from
b74fe7d to
cb793d3
Compare
sijie
pushed a commit
to sijie/pulsar
that referenced
this pull request
Mar 4, 2018
…est endpoint) to WorkerService (apache#239)" (apache#241) This reverts commit 8fd1fd1870753ea8fb2576873800380ac2cea3dd.
hangc0276
pushed a commit
to hangc0276/pulsar
that referenced
this pull request
May 26, 2021
Fixes apache#230 The offset field is not filled when a PRODUCE request is processed yet. It causes that the offset what Kafka producer get from send()'s callback is always -1, which may cause some problems like apache#241 mentioned. This PR fills the offset field and add a unit test to verify if the right offset is set.
hangc0276
pushed a commit
to hangc0276/pulsar
that referenced
this pull request
May 26, 2021
Master issue: apache#241 This PR add support for Kafka CreateTopics request so that topics can be created by Kafka's AdminClient. Related tests are added to verify The validateOnly field introduced from CreateTopics request v1 is not processed. The validateOnly field only checks the topics can be created as specified but not creates anything. It's not a important feature currently so leave it as a future task. * Add handler for CreateTopics request * Fix wrong numTopics and default namespace * Add test for CreateTopics request handler * Handle the corner case when topic name is invalid * Add this to make code more readable
hangc0276
pushed a commit
to hangc0276/pulsar
that referenced
this pull request
May 26, 2021
Master issue: apache#241 Currently if a Kafka Client tries to list all topics, the topic name in response will contain the namespace prefix like "persistent://public/default". However, for topics in the default namespace, they shouldn't contain the prefix because Kafka Client would like to use the short topic name. So this PR will remove the prefix of topics in the default namespace. * Add a method to remove the default namespace prefix * Use short topic name if the topic is in the default namespace * Change variable name to avoid conflict * Fix simpleProduceAndConsumeWithPulsarAuthed test error
hangc0276
pushed a commit
to hangc0276/pulsar
that referenced
this pull request
May 26, 2021
Master issue: apache#241 This PR add support for Kafka DescribeConfigs request. The request is to get topic or broker related configs from Kafka but the Kafka's config is much different from KoP. We just only support request for topic's configs and use the default config value so that Confluent Schema Registry can run with KoP. * Handle DescribeConfig request * Add check for topic existence * Fix NPE when DescribeConfigsRequest has no config names * Check for non-existed topic * Add unit test for DescribeConfigsRequest
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The compatibility with previous versions was lost due to past versions .
Modifications
I readded some classes and methods for compatibility with previous versions.
Result
Previous versions clients can be compiled with latest version's header.