Raising proposal based on PR discussion for: Enhance KafkaBridge resurce with consumer inactivity timeout and HTTP consumer/producer parts enablement

[steffen-karlsson]

No description provided.

[scholzj]

Thanks for the proposal. I left few initial comments.

[steffen-karlsson]

Thanks for the proposal. I left few initial comments.

Comments resolved @scholzj.

[steffen-karlsson]

@scholzj @ppatierno Let me know if you need more from me

[scholzj]

Maybe I should try to explain the proposal process a bit ... the proposal is used to discuss, design, and decide how some more complicated issues will be addressed. Thsi includes typically things that:

• Change the API (the CRDs)
• Might have some techncial consequences, cause future limitations etc.

In this case, the change of the API / CRDs would be the main reason for the proposal. While it is sometimes useful to have a prototype PR written in is not necessarily required. I personally would not link it in the proposal but for example in the comments / PR description as an example of how it might be implemented.

If there are multiple variants -> you would normally choose one and propose it and list the others in the rejected alternatives for others to consider. This could be based on your liking but also on the follow-up discussion. So in this case we should pick one version and use it and move the other to the rejected alternatives. It seems the version two is prefereed by Paolo and probbaly slighly by me as well. So maybe we can pick that one?

Having the one variant selected is important, because at the end, the proposal is voted upon and needs to get at least 3 maintainer votes (approvals) (see more info here). So it needs to be clear what the plan is.

So we should pick the right variant, get the votes which I think should be pretty straigth forward here and then we can proceed with the PRs.

[steffen-karlsson]

@scholzj, thanks for explaining :)
Though it was merged and then discussed after, but that makes sense - my bad.

[k-wall]

@ppatierno the Bridge property name http.timeoutSeconds makes you think that the property applies to both producer and consumer use-cases. Wouldn't http.consumer.timeoutSeconds been a more obvious (self-documenting) choice? I ask this question here because this proposal is about to make this part of the Kube API.

[scholzj]

@k-wall I would try to avoid doing the deep nesting and having consumer sections everywhere. So I would not put it under .soec.http.consumer and should it be related only to consumer, I would go for .spec.consumer.timeoutSeconds. WDYT @ppatierno?

[steffen-karlsson]

@scholzj if timeoutSeconds should be available for both consumer and producer, I think thats a better solution, more in line with version 2 presented.

[scholzj]

@scholzj if timeoutSeconds should be available for both consumer and producer, I think thats a better solution, more in line with version 2 presented.

If it applies to both, it makes more sense under spec.http as you have it I guess. That also coresponds to http.timeoutSeconds as it exists in the bridge itself. But I would defer to @ppatierno as to what he thinks.

[steffen-karlsson]

@scholzj if timeoutSeconds should be available for both consumer and producer, I think thats a better solution, more in line with version 2 presented.

If it applies to both, it makes more sense under spec.http as you have it I guess. That also coresponds to http.timeoutSeconds as it exists in the bridge itself. But I would defer to @ppatierno as to what he thinks.

Sure thing. Sorry for the potential misunderstanding, what I meant was if its relevant for those timeoutSeconds to differ from consumer to producer.

[k-wall]

https://github.com/strimzi/strimzi-kafka-bridge/blob/main/src/main/java/io/strimzi/kafka/bridge/http/HttpConfig.java#L37 shows this property applies only to the consumer. I think should be a change considered against the Bridge itself that would rename the property http.consumer.timeoutSeconds (retaining but deprecating the old name). The API being proposed by this change should be .spec.consumer.timeoutSeconds.

[ppatierno]

The choice is not so obvious :-( and I think that the initial choice of http.timeoutSeconds was bad (sorry for that), because it didn't make it clear that the configuration is for the consumer only (as pointed by @k-wall).
The thing is that the bridge has two parts: http and kafka. But they are not that clear in the KafkaBridge CRD (i.e. consumer.config is about Kafka related parameter so everything like kafka.*, but it's not consumer.kafka.config which could have been better).
Another idea ... I know it's going to use a different order from the application.properties file but what about using consumer.http.timeoutSeconds, consumer.http.enabled, producer.http.enabled ? I know they will be the opposite of what we have in the application properties starting with http.* but we already have the approach of consumer and producer sections in the KafkBridge custom resource.
At the same time it means an additional inner level, which we could avoid.
@scholzj @k-wall @steffen-karlsson wdyt?

[scholzj]

@ppatierno I would try to avoid the deep nesting and the confusing use of http in multiple places. So maybe you could do something like:

spec:
  http:
    consumerEnabled: true
    producerEnabled: true
    consumerTimeoutSeconds: 60

if you think these things belong to HTTP?

[ppatierno]

@scholzj also your suggestion is fine but I am really struggling to take a strong position. I am really regretting the choices made in the past for the current names in the application.properties file.
Regarding the enabled field for example, while it's enabling/disabling the HTTP part of consumer/producer, in reality it's enabling/disabling the consumer/producer as whole (disabling HTTP, no Kafka client is created) so the best should have been just consumer.enabled and producer.enabled (without the http prefix). So the timeout seconds should have been just consumer (if we assume that any other Kafka related timeout should go in the config part).
At this point I would be for having these params directly under consumer (enabled and timeoutSeconds) and producer (enabled) sections.

[steffen-karlsson]

Not sure exactly what you mean by "Also I would avoid the code formatting on "http enablement", "consumer" and "producer" but using it for actual code and fields." @ppatierno

Everything else is updated @ppatierno and @k-wall.

[ppatierno]

Not sure exactly what you mean by "Also I would avoid the code formatting on "http enablement", "consumer" and "producer" but using it for actual code and fields." @ppatierno

The usage of the single apex which provides the formatting like this formatting with single apex is typically for code and yaml fields while you are just mentioning "http enablement", "consumer" and "producer". So I would avoid having something like http enablement, consumer and producer unless you refer to fields in the spec or code.

[k-wall]

lgtm (non-binding)

[ppatierno]

@steffen-karlsson maybe I was not clear but now your removed single apex from everywhere even where it makes sense :-D ... for example http.consumer.timeoutSeconds makes sense having the single apex and formatted as code, but you removed from it having just http.consumer.timeoutSeconds.
As I said, YAML fields should have such format.
Let me suggesting the changes in the places where it makes sense.

[steffen-karlsson]

@steffen-karlsson maybe I was not clear but now your removed single apex from everywhere even where it makes sense :-D ... for example http.consumer.timeoutSeconds makes sense having the single apex and formatted as code, but you removed from it having just http.consumer.timeoutSeconds.
As I said, YAML fields should have such format.
Let me suggesting the changes in the places where it makes sense.

I somehow also thought it would make sense somewhere but I went with the full removal, let me re-add a few 😅

[ppatierno]

I think we can remove this sentece. It's just part of the proposed solution.

[scholzj]

I think Paolo's comments make sense and should be applied. But otherwise it looks good to me.

[ppatierno]

@steffen-karlsson just adding one more thing. The 071 number was just taken by another merged proposal.
You should rename the file using the next available number and also update the README file with this proposal (number and title).

[ppatierno]

LGTM. Thanks for the proposal!

[see-quick]

LGTM. Thanks! 💯

[ppatierno]

@steffen-karlsson as for the other PR, I see a lot of commits which are not related to the proposal. What's happened? Can you try to clean it please? Closing and opening is not good because we'll lost all the approvals we already have so you should fix on this PR imho.

[steffen-karlsson]

@steffen-karlsson as for the other PR, I see a lot of commits which are not related to the proposal. What's happened? Can you try to clean it please? Closing and opening is not good because we'll lost all the approvals we already have so you should fix on this PR imho.

Git happend, tried to update from main due to conflict and then my fork was out of sync.
Let me squash it all into one

[ppatierno]

This proposal has enough votes to be approved and merged (4 binding, 1 non-binding).
Because it's has been opened for long time, I will wait the end of tomorrow (April 16th) and then approve/merge if no further feedback are provided.

[steffen-karlsson]

This proposal has enough votes to be approved and merged (4 binding, 1 non-binding). Because it's has been opened for long time, I will wait the end of tomorrow (April 16th) and then approve/merge if no further feedback are provided.

When its merged, ill start adjusting the linked PRs accordingly

[ppatierno]

This proposal is approved with 4 binding and 1 non-binding votes. Going to merge.
@steffen-karlsson you can even back to work on the implementation PR changing it accordingly. Thanks!