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

Upgrade the Kafka Library to 1.4.3 #151

Merged
merged 10 commits into from
Aug 20, 2020
Merged

Upgrade the Kafka Library to 1.4.3 #151

merged 10 commits into from
Aug 20, 2020

Conversation

TsuyoshiUshio
Copy link
Contributor

I update the Kafka Library to 1.4.3.

It includes big change on the SchemaRegistry.

https://github.com/confluentinc/confluent-kafka-dotnet/releases

Outcome

The New library includes several fix.

  • We don't need ca certificate configuration. v1.1.0
  • NuGet package is signed v1.2.0

What I did

For more details, please refer to References

  • ISchemaRegistryClient interface change

For reviewers

Since the ISchemaRegistryClient interface has been changed, I don't have 100% confidence for this change. It is call back interface so we don't directory reference it. I confirmed all the Unit/E2E test works on my local machine. The implementation of the LocalSchemaRegistry can be the biggest review point.

I'm wondering if I includes this fix to the GA version. People want GA that means official support. I can publish the current version as GA and upgrade it after that. That is all tested, however, if we could have this version as a GA, it might be great.

fbeltrao
fbeltrao previously approved these changes Jul 1, 2020
Copy link
Contributor

@fbeltrao fbeltrao left a comment

Choose a reason for hiding this comment

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

LGTM. Would advice to release as a beta and ask people to give a try.

@fbeltrao
Copy link
Contributor

fbeltrao commented Jul 1, 2020

If proto buff is part of the library does it mean that we can drop the usage of Google.Protobuf?

@TsuyoshiUshio
Copy link
Contributor Author

Hi @fbeltrao
According to your comment, I change the dependency from protobuf to the Confluent.SchemaRegistry.Serdes.Protobuf.
69225d2

@ryancrawcour
Copy link
Contributor

@ahmelsayed / @anirudhgarg can we get a review on this please. LGTM and @fbeltrao. this is related to issue #157 regarding CA certs, so if we can get this change in, then we might avoid having to fix that one.

@TsuyoshiUshio
Copy link
Contributor Author

Hi @fbeltrao
I try to follow your direction, however, some Obsolete method looks used during the E2E testing.
Could you refer to this commit? I rollbacked the change.
2cbc268

Without this implementation, the E2E testing fails at least.
However, the implementation is done by reflection, I saw the call stack, however, I can't identify which code calling the LocalSchemeRegistry.cs. Could you share where it is? If you are ok, I'd like to merge this PR for support library.

@TsuyoshiUshio
Copy link
Contributor Author

Sorry @ryancrawcour
for the late reply. I'm surely close and publish new version soon.

@fbeltrao
Copy link
Contributor

@TsuyoshiUshio , looking at Confluent library they seem to have helpers to define names for keys and values.
We could implement something according the lines:

public class LocalSchemaRegistry : ISchemaRegistryClient
{
    private readonly string schema;
    private readonly SubjectNameStrategyDelegate keyNameStrategy;
    private readonly SubjectNameStrategyDelegate valueNameStrategy;
    private readonly List<string> subjects = new List<string>();

    public LocalSchemaRegistry(string schema, SubjectNameStrategy valueNameStrategy = SubjectNameStrategy.Topic, SubjectNameStrategy keyNameStrategy = SubjectNameStrategy.Topic)
    {
        this.keyNameStrategy = keyNameStrategy.ToDelegate();
        this.valueNameStrategy = valueNameStrategy.ToDelegate();
        this.schema = schema;
    }

    <other members removed for simplicity>

    
    public Task<int> RegisterSchemaAsync(string subject, string schema) => Task.FromResult(1);
    public Task<int> RegisterSchemaAsync(string subject, Schema schema) => Task.FromResult(1);
}

The purpose of the LocalSchemaRegistry is to enables a scenario to use Avro without a schema registry. More work is needed to actually support an schema registry.

@TsuyoshiUshio
Copy link
Contributor Author

Thank you @fbeltrao

Thank you for the point! That is very good to find the delegate.
As we discussed, It might be better to sperate the concern.

I create a new issue for the Local Schema Registry topic. I'll investigate more and work on that issue. :)
#162

@ryancrawcour
Copy link
Contributor

@TsuyoshiUshio if you're separating that work in to a new issue, is this one complete & ready? or still busy with it?

@TsuyoshiUshio
Copy link
Contributor Author

TsuyoshiUshio commented Aug 19, 2020

@ryancrawcour This version works. As the same level as the last version. I mean ready.

The proposal from the @fbeltrao is good one however, not directly related to the version update. So that we decide to go with separate PR.
My plan is Merge this one, then test KafaOutput binding one and EventHub with Kafka API issue based on this version.

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

4 participants