Support stream level configuration

[LaPetiteSouris]

This PR is to add custom stream level configuration in the API, so that upon a CreateStream request, the client has the possibility to customize the stream configuration.

Related issue

[tylertreat]

I'm a little nervous about the use of google.protobuf.Duration based on the notes here. Are we going to run into compatibility issues for clients in languages other than Go? Especially if Go has to use gogo/protobuf to get it to work. Would we be better off just using a primitive value and converting to time.Duration?

[LaPetiteSouris]

I'm a little nervous about the use of google.protobuf.Duration based on the notes here. Are we going to run into compatibility issues for clients in languages other than Go? Especially if Go has to use gogo/protobuf to get it to work. Would we be better off just using a primitive value and converting to time.Duration?

I am also a bit nervous about this aspect, as explained in the notes itself. The main concern is that it may seriously affect other clients. We don't know what we don't know.

I suggest we can switch to use just an int64 to indicates duration in minutes . This kills the convenience of Duration type, but this is the safest way. Better safe than sorry. I can create an issue after the PR to indicate that at some point in time, we would need to improve this feature by switching support to Duration type.

What do you think ?

[tylertreat]

That sounds good to me.

[LaPetiteSouris]

That sounds good to me.

Actually after looking through the code for a while, I was thinking about

1. Duration as string
   putting this field in api.proto as a string. So that the correct format will be like:

1000s
1m
15m

It is easy to parse from this correctly-formated format to time.Duration, and still preserve the flexibility of setting this value in any type of duration we want (minute, second...etc)

2. Bool field as string

For example, Compact configuration is a boolean value on the server. However, if we declare this field in the request's proto as bool, that means when the client submit a CreateStream request, if this field is not set, a false value will always be set by default, thus overwrite the broker's level configuration . This is kind of a bug to me.

What do you think about these suggestion of using string field for Duration and bool field in proto request ?

[tylertreat]

The duration as string is interesting, but I think I'd prefer to keep it as an int64. The string is less predictable, i.e. what formats are supported, ns, ms, s, m, d? Can they be combined? It's not obvious from a user's perspective IMO.

For the compact field, I wonder if this should instead be an enum. The default value is to take the server config and the two other values would be enable/disable.

[LaPetiteSouris]

The duration as string is interesting, but I think I'd prefer to keep it as an int64. The string is less predictable, i.e. what formats are supported, ns, ms, s, m, d? Can they be combined? It's not obvious from a user's perspective IMO.

For the compact field, I wonder if this should instead be an enum. The default value is to take the server config and the two other values would be enable/disable.

int64 seems a solid argument. But that's also mean we need to choose uniquely one time unit to support. E.g: either minute or second, since an int64 itself cannot support both minutes and seconds and other.

It also mean, if we decide to support minute , which is most relevant, no configuration can go lower than 1 minute , which is also not that bad either.

enum for compact looks good. Also, it means that if compact is disabled, the compactMaxGoroutines will not be taken into account on the server side. I will put a remark in the docs for that

[tylertreat]

For time unit, milliseconds int64 is probably sufficient in terms of precision?

[LaPetiteSouris]

For time unit, milliseconds int64 is probably sufficient in terms of precision?

I think milliseconds is kinda a bit "unrealistic" scenario. But it's fine, since it safeguards precision.

[tylertreat]

Seconds is probably more realistic, but what about cases in the future where we have other durations for things requiring more precision? We'll want to be consistent in how we approach time units.

[LaPetiteSouris]

Seconds is probably more realistic, but what about cases in the future where we have other durations for things requiring more precision? We'll want to be consistent in how we approach time units.

changes:

1. all duration configuration is changed to use int64 and consider it as millisecond by default
2. compact setting is changed to use enum with a default UNKNOWN type

[tylertreat]

lgtm, just some minor changes.

[LaPetiteSouris]

As of this version, all custom stream configuration are using NullableType (NullableInt64, NullableInt32 and NullableBool) to clearly mark the difference between user's configuration and server's default values