Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove SCHEMA endpoint and api_url #1270

Merged
merged 1 commit into from
May 22, 2023
Merged

Remove SCHEMA endpoint and api_url #1270

merged 1 commit into from
May 22, 2023

Conversation

piotrpio
Copy link
Collaborator

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
@piotrpio piotrpio requested a review from Jarema May 22, 2023 10:57
@coveralls
Copy link

Coverage Status

Coverage: 85.207% (-0.04%) from 85.25% when pulling 2606f64 on micro-adr-updates into 7c468cd on main.

@levb
Copy link
Contributor

levb commented May 22, 2023

(@piotrpio test failure - unrelated?)

@piotrpio
Copy link
Collaborator Author

@levb yes, unrelated. I'm working on all those flaky tests that we have now.

@piotrpio piotrpio merged commit a4a0c0e into main May 22, 2023
3 of 4 checks passed
@oderwat
Copy link
Contributor

oderwat commented May 25, 2023

I thought that the schema information was a great idea, :( have to rewrite our code to use metadata instead now.

@oderwat
Copy link
Contributor

oderwat commented May 25, 2023

How am I supposed to query micro for the endpoint metadata? It is not in the "INFO". Are we really supposed to use "STATS" for getting the endpoint meta-data? It looks like the EndpointSchema type is not even used anywhere anymore.

@ripienaar
Copy link
Contributor

@piotrpio piotrpio deleted the micro-adr-updates branch May 26, 2023 07:35
@oderwat
Copy link
Contributor

oderwat commented May 26, 2023

@ripienaar this is the service level metadata. I am talking about the endpoint metadata:

cbvb-318EDFE0-0889-4108-860F-8B9DAD3659E2

We use that to auto-discover the request and reply schemas (including their version). This also contains the documentation for these endpoints.

@ripienaar
Copy link
Contributor

There is no per endpoint metadata atm yes so would only be able to use the top level service metadata

we could consider per endpoint metadata as well

@oderwat
Copy link
Contributor

oderwat commented May 26, 2023

What do you mean, there is no endpoint metadata? The code above is in the package. We have been using it for some weeks. Our code relies on that. Adding all the endpoint metadata to the main service makes it clunky. We also were not aware that the API does change so much (removing the endpoint schema info from all the libraries).

@piotrpio
Copy link
Collaborator Author

Hey @oderwat

Sorry to hear you had to rewrite your code, but huge thanks for being an early adopter of micro!

We decided to get rid of SCHEMA and api_url in attempt to make micro as simple and unopinionated as possible. We basically did not want to make any assumptions around how the schema should be passed around and metadata looks like the best place to keep this sort of data.

Right now per-endpoint metadata is included in STATS, INFO contains the global service metadata, that is correct.

About changes in the API - there is a note in the codebase:

// Notice: Experimental Preview
//
// This functionality is EXPERIMENTAL and may be changed in later releases.

Info about this functionality being experimental was also included in release notes when micro first landed - so unfortunately, while we want to release the stable version of micro as soon as possible, we are right now still in the experimental territory.

@oderwat
Copy link
Contributor

oderwat commented May 26, 2023

@piotrpio I am not complaining about changes in experimental code. That is to be expected, and we do appreciate that you design the stuff in the way you do. The product is fantastic.

If the metadata in the endpoints stays, this would be very nice. Having it available through STATS would be enough too.

But I think having the endpoint information in the "INFO" and removing it from the "STATS" would be more logical (as the metadata should not change as "STATS" do).

There is also that the "INFO" lists the "subjects" which I think reflect the "endpoints", so I would probably replace the "subjects" in the "INFO" call to the actual endpoint data consisting of the "subject" and the "meta" per endpoint.

This way, you had the static information (all meta-data) in the info and the stats call does not need to be used for the endpoint metadata recovery.

@piotrpio
Copy link
Collaborator Author

That sounds nice. Let us discuss that (we need parity in all client libraries) and get back to you, but possibly this is what we'll want to do.

@piotrpio
Copy link
Collaborator Author

@oderwat we'll be adding endpoint info (with metadata) to INFO response, and removing endpoint metadata from STATS - nats-io/nats-architecture-and-design#221

@oderwat
Copy link
Contributor

oderwat commented May 30, 2023

@piotrpio LGTM :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants