Configure KafkaCluster dynamically

[slinkydeveloper]

Fix #964, details in individual commits

[slinkydeveloper]

Tested manually and with restatedev/e2e#404

[muhamadazmy]

Thank you @slinkydeveloper for this PR. I left few minor comments. +1 for merging.

[slinkydeveloper]

This one was reviewed and approved, shall we merge it @tillrohrmann ?

[tillrohrmann]

I didn't review the PR in detail but it seems that Azmy took care of it. So from that perspective it's good to go I believe.

One thing that seems to be missing is the release note for this feature. Do we need to update our e2e tests to test this feature? What about documentation updates?

If I recall correctly, then this feature will allow our Cloud users to connect their clusters with a Kafka cluster. How are we gonna spread the word about this new feature/functionality?

How are we handling the situation if there is a conflict between a dynamically and statically configured KafkaCluster? I couldn't answer this questions from the description or the Github commit messages.

[slinkydeveloper]

One thing that seems to be missing is the release note for this feature.

How do i add it, where?

Do we need to update our e2e tests to test this feature?

There is a pending PR in the e2e repo for that: restatedev/e2e#404

What about documentation updates?

I think to complete this feature, we need to do the UI side, which I think would benefit from merging this PR.

How are we gonna spread the word about this new feature/functionality?

It's gonna be in the UI, can't be more clear than that :)

How are we handling the situation if there is a conflict between a dynamically and statically configured KafkaCluster?

It boils down to the fact that you won't be able to register the kafka cluster (see https://github.com/restatedev/restate/pull/4224/changes#diff-d56c5572518c91d8f3ba88179d98fc08c582bd587ecda2fe45a5a894641dc18cR1059). I think, generally, we should phase out configuring kafka clusters from config files, and tell people to use the admin api/ui instead (i would even remove it from the docs for 1.7).

[tillrohrmann]

You can add the release notes in the /release-notes directory following the process described here https://github.com/restatedev/restate/blob/69192d86d2246b792f34869f2cfbfd4ac5e1f0a5/release-notes/README.md#L1-L0.

Should we create an umbrella issue with the missing work items to complete shipping this feature? It seems there is documentation work, UI work, and deprecation work still pending.

It boils down to the fact that you won't be able to register the kafka cluster (see https://github.com/restatedev/restate/pull/4224/changes#diff-d56c5572518c91d8f3ba88179d98fc08c582bd587ecda2fe45a5a894641dc18cR1059).

What happens if there is dynamically registered cluster and someone configures a static Kafka cluster and restarts the node? Would the process crash?

[slinkydeveloper]

What happens if there is dynamically registered cluster and someone configures a static Kafka cluster and restarts the node? Would the process crash?

It won't crash, dynamic takes precedence over static. https://github.com/restatedev/restate/pull/4224/changes#diff-79c9bf5b2cf3b90bca103db3413ba8f1d6efdbc8eadabcb8ad0c2432406c990eR896

Should we create an umbrella issue with the missing work items to complete shipping this feature? It seems there is documentation work, UI work, and deprecation work still pending.

Will do on merging

[slinkydeveloper]

@tillrohrmann release notes added

[slinkydeveloper]

@tillrohrmann can we merge this?

[tillrohrmann]

It won't crash, dynamic takes precedence over static. https://github.com/restatedev/restate/pull/4224/changes#diff-79c9bf5b2cf3b90bca103db3413ba8f1d6efdbc8eadabcb8ad0c2432406c990eR896

Are we telling users about in form of a log message or so?

From my side go ahead merging and creating the umbrella issue for the follow-up tasks.

[slinkydeveloper]

Are we telling users about in form of a log message or so?

I don't think we need to, it's gonna be just spammy... Also where would we even do that? on startup? only on startup it would make sense, but where?

[tillrohrmann]

I don't know the answer to where and when. I am just thinking about the user who accidentally misconfigures things and then can't figure out why their Kafka clusters aren't showing up or different ones show up. Especially since the behavior is asymmetric wrt whether static or dynamic configuration exists first.

[slinkydeveloper]

I don't know the answer to where and when. I am just thinking about the user who accidentally misconfigures things and then can't figure out why their Kafka clusters aren't showing up or different ones show up. Especially since the behavior is asymmetric wrt whether static or dynamic configuration exists first.

Would this concern be reduced if we deprecate the static config?

[slinkydeveloper]

@tillrohrmann i pushed an update adding an Info section to the kafka cluster response (we use the same approach already for services). This will be displayed in the UI

[tillrohrmann]

Thanks for creating this PR @slinkydeveloper. The changes look good to me. I left a few minor comments. +1 for merging after resolving them.