-
Notifications
You must be signed in to change notification settings - Fork 513
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
Add a better worded exception of unsupported SpecificRecord coder case #4815
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4815 +/- ##
==========================================
+ Coverage 62.40% 62.43% +0.03%
==========================================
Files 280 280
Lines 10407 10410 +3
Branches 760 781 +21
==========================================
+ Hits 6495 6500 +5
+ Misses 3912 3910 -2
|
s"Avro schema by instantiating $clazz. Use only a concrete type implementing " + | ||
s"SpecificRecord or use GenericRecord type in your transformations if a concrete " + | ||
s"type is not known in compile time." | ||
throw new Throwable(msg, cause) |
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.
I think this isn't worth chaining the exception here, as the one raised by
SpecificData.get().getSchema(clazz)
is already suppressed. We're already in a fallback/recover case.
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.
- my intuition is that this extra info won't hurt
- this would make clear if failure happens for anything other than "constructor not found", like
getSchema
might fail because of some bug in apache avro
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.
Ok. I'd also change the exception type with a IllegalArgumentException
and if using the try/cath
, prefer
case NonFatal(e) =>
instead of catching any Throwable
// Otherwise create a default instance and call getSchema | ||
.getOrElse(getSchemaFromEmptyInstance) |
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.
orElse
will make this more idiomatic IMHO
// Otherwise create a default instance and call getSchema | |
.getOrElse(getSchemaFromEmptyInstance) | |
.orElse(Try(clazz.getDeclaredConstructor().newInstance().getSchema)) | |
.getOrElse { | |
val msg = ... | |
throw new RuntimeException(msg) | |
} |
No description provided.