-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[HUDI-7966] Handle NPE from AvroSchemaUtils.createNewSchemaFromFieldsWithReference #11585
Conversation
726f012
to
ea163f8
Compare
newSchema.addProp(prop.getKey(), prop.getValue()); | ||
} | ||
} else { | ||
LOG.warn("Schema.getObjectProps() returned null for schema: {}", schema); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we even need to log a warn ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we throw here, what is the schema without any fields there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema has fields but no props. Throwing error would result in the same failure. The props were copied in eb20273. We need to understand what's the side-effect of not having props. I think schema fields should be sufficient. @jonvex can help clarify this point.
do we even need to log a warn ?
Logging a warn for debugging purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I found the doc mentioned the attributes: https://avro.apache.org/docs/1.11.1/specification/#schema-declaration, it should not affect the serialization/deserialization, but it doesn't say how it is affected.
ea163f8
to
a724237
Compare
Change Logs
Running long-running deltastreamer with following properties: https://github.com/apache/hudi/blob/dbfe8b23c0b4f160b26379053873cfc2a46acef4/docker/demo/config/test-suite/test-nonpartitioned.properties
The job throws NPE during validation phase:
The code assumes that all schema must have properties, which may not necessaily be true. This PR handles NPE by simply logging a warning if props are not present. Fields are still added (no behavior changes) and if props are present then those are added too.
Impact
Bug fix, avoid blocking ingestion.
Risk level (write none, low medium or high below)
low
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist