[schema registry] Return undefined for not found #15149
Conversation
nguerrera
force-pushed
the
undefined-for-schema-not-found
branch
from
4eae23c to
73512cc
Compare
nguerrera
force-pushed
the
undefined-for-schema-not-found
branch
from
73512cc to
4a1af31
Compare
nguerrera
force-pushed
the
undefined-for-schema-not-found
branch
from
4a1af31 to
a41344d
Compare
nguerrera
requested review from
bterlson,
chradek,
ramya-rao-a and
xirzec
as code owners
| @@ -167,6 +167,10 @@ export class SchemaRegistryAvroSerializer { | |||
| } | |||
|
|
|||
| const schemaResponse = await this.registry.getSchemaById(schemaId); | |||
| if (!schemaResponse) { | |||
| throw new Error(`Schema with ID '${schemaId}' not found`); | |||
There was a problem hiding this comment.
could possibly use RangeError for this
There was a problem hiding this comment.
If this is a common scenario that would need to be handled programmatically, we could make a service-specific error with a custom name that devs can look for instead of having to introspect the message
There was a problem hiding this comment.
My gut feeling is that this isn't an error you would want to handle in a unique way. I think your architecture should be such that you take the necessary steps to have the needed schema in the registry before trying to serialize/deserialize and failure to do so could be considered a mistake by the caller and not something they should handle, or handle any differently from any other failure to serialize/deserialize (apart from logging the different specifics in the messages somewhere).
But I'm not 100% sure on this. @dhoepelman, do you also use the avro serializer package? What would you like to see from it when the schema can't be matched in the registry?
There was a problem hiding this comment.
We use the package yes
You are correct, this is in the deserialization codepath and at that time the schema ID not found is totally unexpected and unrecoverable, you would need to do unreasonable things like removing the schema while messages with it are still on the hub to encounter it.
At serialization time either autoRegister should be true and it can't occur or the schema should already be registered before serializing and its a unrecoverable caller error.
As in both cases programmatical recovery different from any other error is not possible, a specific error seems unnecessary.
There was a problem hiding this comment.
Thank you for confirming!
| } else { | ||
| const response = await this.registry.getSchemaId(description); | ||
| if (!response) { | ||
| throw new Error("Schema not found in registry."); |
There was a problem hiding this comment.
is there any meaningful way we can refer to the schema to provide better logging?
There was a problem hiding this comment.
Yes, there is a name I should have used. Will fix.
There was a problem hiding this comment.
It could be confusing though because the name could match but the content differs and this throws. Will think about how to word this.
There was a problem hiding this comment.
I'm thinking something like "Schema with name <name> not found in registry group <group>, or not found to have matching content." A little awkward, so any thoughts on a better wording are most welcome.
There was a problem hiding this comment.
The schema content could also be useful in debugging this, but I think it would be too long to include in the exception message in the common case. Also, it's still an arg you passed in so you could always log/trace it yourself.
There was a problem hiding this comment.
I think content would be too verbose, though if you really wanted to you could make a custom error type to attach it as a property. Not really worth it if it's not being programmatically handled though.
|
@xirzec I pushed an update to the error message to include name and group, and I think after the discussion this was the only thing left to do, but just double checking and giving you a chance to review the commits after your sign-off before I merge... |
| const response = await this.registry.getSchemaId(description); | ||
| if (!response) { | ||
| throw new Error( | ||
| `Schema '${description.name}' not found in registry group '${description.group}', or not found to have matching content.` |
There was a problem hiding this comment.
Since you asked for an alternative string, here's my best attempt, but don't feel like you have to take it. 😄
| `Schema '${description.name}' not found in registry group '${description.group}', or not found to have matching content.` | |
| `Can't locate schema '${description.name}' in registry group '${description.group}' from given description. Did you mean to set autoRegisterSchemas?` |
There was a problem hiding this comment.
Thanks. Elsewhere we say to avoid autoRegisterSchemas in production based on guidance from the service team. So I think we shouldn't point to it here. Leaving the message as I have it given that.
Fix #15130
This turns 404 into undefined for SchemaRegistryClient.getSchemaId and SchemaRegistryClient.getSchemaById.
Note, however, that the higher level avro serializer still throws on not found. While it feels natural enough when you're interacting directly with the schema registry to say "can I have a schema that matches this description or id" and get back undefined for "sorry no such schema", there is no natural return value for when you try to serialize/deserialize with a schema that is not found in the registry, so I think this should remain an exception, but I'm open to different design ideas.