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

docs: klip-41: ksqlDB .Net Client #6613

Merged
merged 4 commits into from
Nov 24, 2020

Conversation

alex-basiuk
Copy link
Contributor

Description

A proposal for a .Net client

Testing done

N/A

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@alex-basiuk alex-basiuk requested a review from a team as a code owner November 13, 2020 23:49
@ghost
Copy link

ghost commented Nov 13, 2020

@confluentinc It looks like @alex-basiuk just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@agavra
Copy link
Contributor

agavra commented Nov 24, 2020

Thanks @alex-basiuk - this is really exciting stuff. We haven't really come up with a good way to structure development around ksqlDB clients. Did you have any idea about where you were planning on having the code live? Do you have a prototype or code hosted somewhere publicly that you want us to review? A good way forward on this (the KLIP LGTM) would be to start development as an independent client and then when it gains traction popularity, we can bring it under the ksqlDB "official" umbrella of clients and in the meantime, we can point to it from our README to help get it some traction.

@alex-basiuk
Copy link
Contributor Author

Thanks @alex-basiuk - this is really exciting stuff. We haven't really come up with a good way to structure development around ksqlDB clients. Did you have any idea about where you were planning on having the code live? Do you have a prototype or code hosted somewhere publicly that you want us to review? A good way forward on this (the KLIP LGTM) would be to start development as an independent client and then when it gains traction popularity, we can bring it under the ksqlDB "official" umbrella of clients and in the meantime, we can point to it from our README to help get it some traction.

Thanks @agavra, it's not publicly available yet, but will be soon. I'll post updates here and will add a link to the README.
One thing which is not entirely clear to me is how are you going to evolve your REST API. You have a notion of version in your schema e.g. application/vnd.ksql.v1+json. I guess you'll create new versions in future. But you also support application/json which is not versioned at all.

## What is in scope

* A new client implemented in C#.
* Initially, the client will be targeted at .NET Core 3 and .NET Framework 5. Support of older versions will be added later.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not versed in .NET, but one thing I'm wondering is how support for multiple versions will look over time. Any ideas about how to manage that workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichaelDrogalis .NET 5 and .NET Core 3 are very close in terms of APIs. I don't think .NET 5 has any breaking changes in the libraries required by this project. It won't be hard to support both until end of support of .NET Core 3.1. Basically, you can have one project which targets multiple framework versions.
Support of older versions is probably more involved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, that makes sense. Mostly just asking since Im unfamiliar with the .NET stack.

such as list and describe.
* The client will utilise C# [async streams](https://docs.microsoft.com/en-us/dotnet/csharp/tutorials/generate-consume-asynchronous-stream) feature.
* The client will also support a direct / synchronous interaction style.
* High-quality documentation and examples.
Copy link
Contributor

Choose a reason for hiding this comment

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

Super. :) If this is going to live inside of a repo owned by you for a while, we can put the documentation in a README there so it is self-contained.

## Test plan

Unit and integration test will be mandatory. Performance benchmarks are desirable but unlikely be the first priority.
Ideally, tests should be executed on both Windows and Linux platforms.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do any CI providers make dual Windows tests easy? Been a while since I've tested much on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichaelDrogalis Travis, CircleCI, Azure CI. Folks from confluent-kafka-dotnet use Travis.

@agavra
Copy link
Contributor

agavra commented Nov 24, 2020

One thing which is not entirely clear to me is how are you going to evolve your REST API. You have a notion of version in your schema e.g. application/vnd.ksql.v1+json. I guess you'll create new versions in future. But you also support application/json which is not versioned at all.

cc @vcrfxia might have some input on this, I'm not totally sure what our versioning strategy for the client is going forward

@agavra agavra merged commit 2b80379 into confluentinc:master Nov 24, 2020
@vcrfxia
Copy link
Contributor

vcrfxia commented Dec 3, 2020

One thing which is not entirely clear to me is how are you going to evolve your REST API. You have a notion of version in your schema e.g. application/vnd.ksql.v1+json. I guess you'll create new versions in future. But you also support application/json which is not versioned at all.

cc @vcrfxia might have some input on this, I'm not totally sure what our versioning strategy for the client is going forward

application/json refers to the latest version at any given point in time:

if (mediaType == null || MediaType.APPLICATION_JSON.equals(mediaType)) {
return KsqlMediaType.LATEST_FORMAT;
}

Currently the latest (and only) version is application/vnd.ksql.v1+json:
public static final KsqlMediaType LATEST_FORMAT = KSQL_V1_JSON;

@alex-basiuk
Copy link
Contributor Author

Thanks @vcrfxia
Basically, it means that if I use application/json in my application, it can break at any time when you upgrade services on your side to a newer version. Why do you need it at all?

Also, I noticed that the /ksq endpoint return @type property which is not documented. It would be cleaner to remove it.

@vcrfxia
Copy link
Contributor

vcrfxia commented Dec 8, 2020

Basically, it means that if I use application/json in my application, it can break at any time when you upgrade services on your side to a newer version. Why do you need it at all?

The decision to include this option predates me. Perhaps @rodesai has context?

Also, I noticed that the /ksql endpoint return @type property which is not documented. It would be cleaner to remove it.

Thanks for the feedback. The field comes from ksqlDB's use of Jackson subtypes for server response types. ksqlDB 1.0 will likely include pretty significant changes to the server API endpoints. That would be the time to reconsider this, as removing an existing field would be a breaking change.

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