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

JetStream API clean-up #212

Merged
merged 8 commits into from
Nov 16, 2023
Merged

JetStream API clean-up #212

merged 8 commits into from
Nov 16, 2023

Conversation

mtmk
Copy link
Collaborator

@mtmk mtmk commented Nov 15, 2023

  • Removed opinionated method overloads for create consumer and stream.
  • Removed publish overload for sending empty messages. Doesn't feel like a common operation in JS. Removed to reduce the API footprint.
  • A few format fixes.
  • A couple of potential test flapper timeout fixes.

/cc @Jarema

* Removed opinionated method overloads for create consumer and stream.
* Removed publish overload for sending empty messages. Doesn't feel like
  a common operation in JS. Removed to reduce the API footprint.
* A few format fixes.
* A couple of potential test flapper timeout fixes.
@mtmk mtmk requested a review from caleblloyd November 15, 2023 21:00
@mtmk mtmk requested a review from piotrpio November 15, 2023 22:17
@mtmk mtmk mentioned this pull request Nov 15, 2023
@caleblloyd
Copy link
Collaborator

I do miss the simplicity, it's no longer clear what settings are actually required. Would it make sense to provide some static helpers/extensions

// helpers
static StreamConfig(string streamName, IEnumerable<string> subjects) StreamCreateRequest
static ConsumerConfig(string streamName, string consumerName) ConsumerCreateRequest

// extensions
static ConsumerConfig(INatsJetStream streamName, string consumerName) ConsumerCreateRequest

That way if I want an R3 stream I could do

var sc = new StreamConfig("my-stream", []{"my-stream"}) with {Replicas = 3}

Much simpler in my opinion than having to look into the detials of the record to discover that Name and Subjects are required for me to actually get a working stream

Also - not sure how often people create Ephemeral consumers explicitly. Might be worth making the helpers explicit for DurableConsumerConfig and EphemeralConsumerConfig. Ephemeral Consumers in that case could have random names.

It takes some deep knowledge to know that you must supply Name and DurableName

@caleblloyd
Copy link
Collaborator

Any interest in shortening the models ConsumerConfiguration => ConsumerConfig and StreamConfiguration => StreamConfig... And any other model with Configuration => Config... It is a long word 😄

Copy link
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

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

LGTM

@Jarema
Copy link
Member

Jarema commented Nov 16, 2023

I like the change to Config!

@mtmk mtmk requested a review from caleblloyd November 16, 2023 18:47
# Conflicts:
#	src/NATS.Client.ObjectStore/NatsObjContext.cs
@mtmk mtmk merged commit d3276bd into main Nov 16, 2023
9 checks passed
@mtmk mtmk deleted the js-api-clean-up branch November 16, 2023 19:10
@@ -19,7 +19,7 @@

var js = new NatsJSContext(nats);

var consumer = await js.CreateConsumerAsync(new ConsumerCreateRequest { StreamName = "s1", Config = new ConsumerConfiguration { Name = "c1", DurableName = "c1", AckPolicy = ConsumerConfigurationAckPolicy.@explicit } });
var consumer = await js.CreateConsumerAsync("s1", new ConsumerConfiguration { Name = "c1", DurableName = "c1", AckPolicy = ConsumerConfigurationAckPolicy.@explicit });
Copy link

Choose a reason for hiding this comment

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

Framework guidelines dictate that enum names should be Pascal cased -- then you could avoid the ugly at escape on explicit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately I had an issue with the JSON serializer not using the attributed name then had to make it same case as the JSON property to make it work.

Copy link

Choose a reason for hiding this comment

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

For S.T.Json, the way I do this [net8] is:

builder.Services
   .AddJsonOptions(
        options =>
        {
            options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter<PassportIssuingSettings>(JsonNamingPolicy.CamelCase));
            options.JsonSerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
            
            options.JsonSerializerOptions.TypeInfoResolver = PassportJsonSerializationContext.Default;

Copy link

Choose a reason for hiding this comment

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

PassportJsonSerializationContext is my source generator context for my structures. I'd imagine you guys are using that code path too?

Copy link

Choose a reason for hiding this comment

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

Here's the partial context I declared:

[JsonSerializable(typeof(Passport))]
[JsonSerializable(typeof(PassportIssuingSettings))]
[JsonSerializable(typeof(Visa))]
[JsonSerializable(typeof(ValidationProblemDetails))]
[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)]
public partial class PassportJsonSerializationContext : JsonSerializerContext
{
}

This is for AOT ready serialization etc.

Copy link

@oising oising Nov 20, 2023

Choose a reason for hiding this comment

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

Tbh, some of my declarations are probably redundant - anything in the AddJsonOptions setup can probably be applied to the context instead. I went through an evolution making things work from net6 to net8 :/ - but it works. My enums are serialized to camel case, declared in dotnet as pascal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we have a context, that attribute solution looks promising. Do you still have to set the option?

Copy link

@oising oising Nov 20, 2023

Choose a reason for hiding this comment

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

I think the option is only for when you don't use a context. I started out that way, then added a context later but never took away the option. I assume the attributes on the context are enough :) But maybe the converter registration only works in options. A quick experiment on your side should tell you quickly enough.

Copy link

Choose a reason for hiding this comment

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

Oh, so you can pass in options director to the context ctor too:

_jsonSerializationContext = new PassportJsonSerializationContext(
    new JsonSerializerOptions
    {
        PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
        Converters = {  }
    });

Then you don't need the entry point AddJsonOptions fluff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm. it doesn't seem to work (see example https://github.com/mtmk/example-json-enum)

Need to implement a convertor I think.

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.

4 participants