Adding translated.from annotation when using SchemaTranslator
#970
Conversation
| if (!ConfigurableSchemaComparator.equals(avroSchemaFromEmbedded, avroSchemaFromJson, | ||
| new SchemaComparisonConfiguration(true, true, true, false, true, true, | ||
| Collections.singleton((TRANSLATED_FROM_SOURCE_OPTION))))) { |
There was a problem hiding this comment.
This is a bit hard to read. Why can you provide a constructor in ConfigurableSchemaComparator to allow configure what options to be excluded? Also why DATA_PROPERTY above is not handled simiarly?
There was a problem hiding this comment.
This check will never fail for DATA_PROPERTY because the code above this check adds it just before the check.
Object embededSchemaPropertyVal = avroSchemaFromJson.getObjectProp(DATA_PROPERTY);
if (embededSchemaPropertyVal != null)
{
avroSchemaFromEmbedded.addProp(DATA_PROPERTY, embededSchemaPropertyVal);
}Updated the SchemaComparisonConfiguration call
| if (dataSchema instanceof NamedDataSchema) { | ||
| NamedDataSchema namedDataSchema = (NamedDataSchema) dataSchema; | ||
| // Add annotation if not already present | ||
| if (!namedDataSchema.getProperties().containsKey(TRANSLATED_FROM_SOURCE_OPTION)) { |
There was a problem hiding this comment.
what should you do if existing translated_from prop is different from updated one?
There was a problem hiding this comment.
It doesn't update the source in that case. So, if schema A translates to B, and B is translated to C,
the translatedFrom option in C will be A
There was a problem hiding this comment.
My original question is talking about an error case, I am guessing that should never happen and we should throw error?
But your response provides me something different from my understanding, I thought that annotation is just for direct source, seems not?
There was a problem hiding this comment.
We don't need to stop users from translating a translated schema again, as it is allowed right now in SchemaTranslator right now, and stopping it might fail users.
However, we can use this annotation to fail other flows like PDL -> Proto or Avro -> proto.
There was a problem hiding this comment.
Based on this logic, translated.from annotation is used to indicate the root source, not directly translated source, is this clearly communicated?
| } | ||
| dataSchema = namedDataSchema; | ||
| } | ||
| return dataSchema; |
There was a problem hiding this comment.
this will have side effect of changing passed DataSchema?
There was a problem hiding this comment.
there should be none, except one additional json property in the result avro schema, which is the requirement of this change. This new property will identify the avro schemas as a translated schema.
li-ukumar
changed the title
li.data.translated.from annotation when using SchemaTranslatorschema.translated.from.src annotation when using SchemaTranslator
| @@ -53,6 +58,7 @@ public class SchemaTranslator | |||
| private static final Logger log = LoggerFactory.getLogger(SchemaTranslator.class); | |||
|
|
|||
| public static final String DATA_PROPERTY = "com.linkedin.data"; | |||
| public static final String TRANSLATED_FROM_SOURCE_OPTION = "schema.translated.from.src"; | |||
There was a problem hiding this comment.
will translated.from enough?
There was a problem hiding this comment.
Yes. any unique key will work.
I'll update to translated.from
| if (dataSchema instanceof NamedDataSchema) { | ||
| NamedDataSchema namedDataSchema = (NamedDataSchema) dataSchema; | ||
| // Add annotation if not already present | ||
| if (!namedDataSchema.getProperties().containsKey(TRANSLATED_FROM_SOURCE_OPTION)) { |
There was a problem hiding this comment.
My original question is talking about an error case, I am guessing that should never happen and we should throw error?
But your response provides me something different from my understanding, I thought that annotation is just for direct source, seems not?
li-ukumar
changed the title
schema.translated.from.src annotation when using SchemaTranslatortranslated.from annotation when using SchemaTranslator
|
It was the description for the initial PR. The unit tests are updated now. Updated the |
What
Adding an annotation in translated schemas, translated through SchemaTranslator
Why
This will help in figuring out whether a schema is translated or source of truth.
This annotation can help in determining different behavior for SoT and translated schemas
Test
Updated Unit tests