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
Fix writing/encoding of GenericJsonRecord #9608
Conversation
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.
LGTM
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.
Thanks for fixing this, left one comment.
@@ -36,7 +38,11 @@ public GenericJsonWriter() { | |||
@Override | |||
public byte[] write(GenericRecord message) { | |||
try { | |||
return objectMapper.writeValueAsBytes(((GenericJsonRecord)message).getJsonNode().toString()); | |||
ByteArrayOutputStream buffer = new ByteArrayOutputStream(); |
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 wonder if there's any specific reason we want to create our own ByteArrayOutputStream & JsonGenerator, which is what objectMapper.writeValueAsBytes() is doing internally, instead it is using a ByteArrayBuilder which is basically Jackson's implementation of OutputStream and claim to be more efficient in many of its use case.
I think we should just use objectMapper.writeValueAsBytes() unless we want to write output to somewhere else(not a byte[]).
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.
+1 to what @MarvinCai said
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.
@MarvinCai thanks for the heads up. I was overdoing it. :) I pushed a fix where I simply dropped .toString()
, as you had already pointed out in the issue report.
1e1fcc8
to
4816f73
Compare
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.
LGTM
/pulsarbot run-failure-checks |
* Fix writing/encoding of GenericJsonRecord - Fixes apache#9605 * Add test for JSONSchema encode/decode round-trip (cherry picked from commit 41e118a)
Fixes #9605
Motivation
See #9605
Modifications
JsonNode
object directly to Jackson's ObjectMapper.writeValueAsBytes method instead of calling.toString()
on theJsonNode
value.