Fetch already published schemas by Subject and Version (or latest)#8
Fetch already published schemas by Subject and Version (or latest)#8tak30 wants to merge 4 commits intoklarna:masterfrom
Conversation
k32
left a comment
k32
left a comment
There was a problem hiding this comment.
Hello,
Thanks for the PR, looks like this feature is missing. But perhaps it should be implemented as part of maybe_download/1 function.
src/avlizer_confluent.erl
Outdated
| get_schema(Subject) -> | ||
| get_schema(Subject, "latest"). | ||
|
|
||
| get_schema(Subject, Version) when is_integer(Version)-> |
There was a problem hiding this comment.
Coding style: extra space before when and missing space before ->
Fixed code style and other bugs after code review.
Hello, thanks for reviewing! In my use case, as I didn't register the schema, I don't know the schemaId or the Fingerprint. I do believe that this use case is out of the main flow supported by this library so it should be an independent function, as it's just an extra. |
There was a problem hiding this comment.
Why not cache it, if the caller of this new API can benefit from having it cached.
e.g. so it can crash then restart without having to re-download.
The cache key is currently either regid() or {Subject, FP}.
We can expand the cache key to support {Subject, Version}
however, the current patterns are
-define(IS_REGID(Id), is_integer(Id)).
-define(IS_NAME_FP(NF), (is_tuple(Ref) andalso size(Ref) =:= 2)).
-define(IS_REF(Ref), (?IS_REGID(Ref) orelse ?IS_NAME_FP(Ref))).
I guess we'll have to add a tuple of size 3 for this new cache.
| {ok, #{<<"schema">> := Schema, <<"id">> := Id}} -> | ||
| {ok, Id, Schema}; | ||
| Error -> | ||
| {error, Error} |
| URL = RegistryURL ++ "/subjects/" ++ Subject ++ "/versions/" ++ Version, | ||
| case httpc_download(URL) of | ||
| {ok, #{<<"schema">> := Schema, <<"id">> := Id}} -> | ||
| {ok, Id, Schema}; |
There was a problem hiding this comment.
I think it's more useful to decode it to regid() (change the type spec also).
If you guys believe this flow should be part of the app then yes, let's make the proposed changes. I'll take care of it. |
|
Sorry for late reply. zmstone is right about caching. Other interfaces of this library imply that caching is present, so it's good to keep everything consistent. |
|
Pablo López Viqueira seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
4 participants
Sometimes there is a need to download an already published schema by Subject and Version.
I didn't add the schemas to the local cache as, as far as I understand, in this use case the user of the library should be the one taking care of how to use the schema.