[Merged by Bors] - feat: add support for cluster metadata

[matheus-consoli]

Add support for custom metadata in the cluster configuration.

[cluster.my_favorite_cluster]
endpoint = "127.0.0.1:9003"

[cluster.my_favorite_cluster.metadata.installation]
"fluvio.io/cluster-type" = "local-k8"

This will be important for other tasks, such as #3620.

These changes are retrocompatible.

Closes #3627

[sehz]

This just adds more noise.

Instead add cluster type to config object so can avoid parsing error

[sehz]

[cluster.my_favorite_cluster]
endpoint = "127.0.0.1:9003"
type = "local-k8"

[matheus-consoli]

Instead add cluster type to config object so can avoid parsing error

That would work for the cluster-type issue, but I think @galibey also has a use case for this same feature (cmiiw @galibey)

[sehz]

then lets' add more properties as necessary. annotations doesn't buy anything except adding more friction and increase complexity

[sehz]

if there are more properties to add, then it probably make sense to move profile as separate crate to make it more reusable

[digikata]

There is a tradeoff between hard coded properties and annotations. One benefit of annotation is that there is less coupling between different versions later, and less maintenance updates.

[nacardin]

Why not add this under the Profile struct instead? It would be easier to match to a profile if it was at the Profile level. Putting it at the FluvioConfig level requires querying the cluster reference while iterating through profiles.

[sehz]

Overall LGTM. minor nit

[galibey]

What is the point of having toml::Value field? It is not so much better than just vec of bytes, it is too loose type in my opinion. Plus, we couple our serialization format (toml) to public API.
If we choose to no go with strictly type fields in the cluster config, we could use some MetadataAnnotation enum that encapsulates different possible representations there and we expose metadata: Vec<MetadataAnnotation> instead of pub metadata: Option<toml::Value>

[sehz]

This is similar to the approach cargo metadata: https://docs.rs/cargo_metadata/0.18.1/cargo_metadata/struct.Metadata.html. The whole point of annotation is to add opinionated config to config without adding more dep to the Fluvio crate. It's the same problem, even if this moves to a separate crate. Then, it is the responsibility of the reader/producer of annotation to ensure the config is correct. With serde Value, they can choose to use strong type or not.

[sehz]

LGTM

[matheus-consoli]

bors r+

[bors]

Build failed:

• Done

[matheus-consoli]

the first attempt failed because of a socket timeout, trying again

bors r+

[bors]

Build failed:

• Done

[galibey]

@sehz Imagine you need access to some metadata item from 3 different crates: you have to have serde_toml in all these three places, and you have to do deserialization there, also, you need that target type dependency (or use code duplication). So, it will be something like let my_type: MyType = serde_toml::from_value(config.metadata) everywhere. It is not convenient at all.

[matheus-consoli]

@galibey I think this is a valid concern, what do you think if we add a way for querying metadata?

I think it would look something like pub fn query_metadata<'de, T: Deserialize<'de>>(&self, query: &str) -> Option<T>, with a micro-dls to target specific fields from some metadata table.

for example, consider the table below and a query to get "value":

[cluster.name.metadata.deep.nested]
key = "value"

assert_eq!(cluster.query_metadata::<&str>("deep.nested[key]"), Some("value"));

If we agree on this, it will target all your concerns to some extent:

• no need to add toml as a dependency everywhere
• (in some cases) we don't need the target type or code duplication
  • we can query specific fields, or deserialize them into some map-structure
• is more convinient

some downsides:

• cost of search
• stringly-typed

[galibey]

@matheus-consoli Yes, that would work. I like the idea.

[galibey]

We will also need a method for inserting a metadata entry.

[sehz]

Let's not add more complexity to this API and more work. If some client wants to add strong typing, they can create a wrapper around it which at that point, can hide all dependency.

I agree with @galibey, inserting and updating is needed.

[matheus-consoli]

bors r+

[bors]

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• Done

[galibey]

This is not good. If there is no metadata, the querying here will shut my process.

[matheus-consoli]

ohno, you're right, I missed that one!