Why does GetLatestSchema invalidates the cache ?

[dstendardi]

Hello @riferrei !

Thank you for this library! it's been very useful and we are hoping to contribute to it!

the go doc says :

In order to ensure consistency, we need to temporarily disable caching to force the retrieval of the latest release from Schema Registry.

AFAIU the java serializer does cache the schema even when looking up the latest schema version

Could you explain a bit more why you decided to invalidate the cache for this use case? We are considering using this library in our producers however given the throughput we aim it feels not right to hit the schema registry on every call :)

What other method would you use to satisfy that use case ?

[riferrei]

Hi @dstendardi,

I think the short answer is that I was solving a particular problem at the time that required me to invalidate the cache. For my use case, I needed to read the very last copy of the schema. Once the project became open, I forgot to revisit this design decision 😅

I can create a patch for the code where the invalidation becomes optional. It may take a few days, though.

Ideally, we could provide an overloaded version of the function GetLatestSchema() so the mental model would be the same, but unfortunately, Go doesn't support function overload. I prefer to keep the same function name and signature, so others won't disrupt their code once they upgrade. So the option would be coming up with a new function that has the same signature:

func (client *SchemaRegistryClient) GetLatestSchemaFromCache(subject string, isKey bool) (*Schema, error)

What do you think?

[dstendardi]

Hi Ricardo !

I can create a patch for the code where the invalidation becomes optional. It may take a few days, though.

We can contribute!

So the option would be coming up with a new function that has the same

This option looks definitely better than forcing cache invalidation, however, this raises another question 🙏 : Why not using the cachingEnabled global option?

➕ It would keep the API simple ➕ behavior consistent with the JVM implementation ➖ behavior BC break - can be addressed with a proper changelog

Unless there is a blind spot I would favor KISS and rely on the cachingEnabled option for the specific case where one needs to get the very last version of a schema.

[riferrei]

Why not using the cachingEnabled global option?

What do you mean by a global option?

[AtakanColak]

@riferrei I am considering rewriting the whole module to remove the isKey arguments, add options to pass down httpClient and introduce testing with schema registry containers. What would you think about that?

[dstendardi]

@riferrei

What do you mean by a global option?

Rather than forcing the behavior in the function body, let the caller decide if he wants to use the library with our without cache :

sr := CreateSchemaRegistryClient{}
sr.CachingEnabled(false)

// do something without cache
sr.GetLatestSchema()

and we remove this block :

func (client *SchemaRegistryClient) GetLatestSchema(subject string, isKey bool) (*Schema, error) {

-       // In order to ensure consistency, we need
-      // to temporarily disable caching to force
-      // the retrieval of the latest release from
-      // Schema Registry.
-      cachingEnabled := client.getCachingEnabled()
-      client.CachingEnabled(false)
    concreteSubject := getConcreteSubject(subject, isKey)
    schema, err := client.getVersion(concreteSubject, "latest")
-       client.CachingEnabled(cachingEnabled)

    return schema, err
}

This way the signature is backward compatible, and for those who really need to disable the cache, they can still turn off the option when they instantiates the client.

[riferrei]

This way the signature is backward compatible, and for those who really need to disable the cache, they can still turn off the option when they instantiates the client.

I liked it @dstendardi. If you could file a PR for this, I would be more than happy to review and approve.

[riferrei]

@riferrei I am considering rewriting the whole module to remove the isKey arguments, add options to pass down httpClient and introduce testing with schema registry containers. What would you think about that?

Hey @AtakanColak, I think overall it is a great idea, and I support it. I have a question, though, about the why of removing the isKey. Do you think there aren't many people using SR to store schemas for keys? Or is it something else?

[AtakanColak]

@riferrei I am considering rewriting the whole module to remove the isKey arguments, add options to pass down httpClient and introduce testing with schema registry containers. What would you think about that?

Hey @AtakanColak, I think overall it is a great idea, and I support it. I have a question, though, about the why of removing the isKey. Do you think there aren't many people using SR to store schemas for keys? Or is it something else?

Someone had to add arbitrary functions to work around isKeys arguments, I think adding a prefix shouldn't be the standard in the package whilst developer can simply add it before using srclient.

[riferrei]

Someone had to add arbitrary functions to work around isKeys arguments, I think adding a prefix shouldn't be the standard in the package whilst developer can simply add it before using srclient.

I am sold. Indeed, this shouldn't be the rule but the exception. PR this so hard, my friend ✅

[dstendardi]

@riferrei ☝️ here is a MR :)

[riferrei]

Thank you, @dstendardi 🥳

[AtakanColak]

Closed as it is now merged