You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/02/17 10:59:11 UTC
[GitHub] [pulsar] lhotari opened a new pull request #9608: Fix writing/encoding of GenericJsonRecord
lhotari opened a new pull request #9608:
URL: https://github.com/apache/pulsar/pull/9608
Fixes #9605
### Motivation
See #9605
### Modifications
- use the correct Jackson API to write a JsonNode to bytes
- This bug was in GenericJsonWriter. However since there wasn't similar test coverage for JSONSchema.encode, some test coverage was added for JSONSchema.encode/decode in this same PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #9608: Fix writing/encoding of GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #9608:
URL: https://github.com/apache/pulsar/pull/9608#discussion_r578605935
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/GenericJsonWriter.java
##########
@@ -36,7 +38,11 @@ public GenericJsonWriter() {
@Override
public byte[] write(GenericRecord message) {
try {
- return objectMapper.writeValueAsBytes(((GenericJsonRecord)message).getJsonNode().toString());
+ ByteArrayOutputStream buffer = new ByteArrayOutputStream();
Review comment:
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] MarvinCai commented on pull request #9608: Fix writing/encoding of GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on pull request #9608:
URL: https://github.com/apache/pulsar/pull/9608#issuecomment-781559476
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #9608: Fix writing/encoding of GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9608:
URL: https://github.com/apache/pulsar/pull/9608#discussion_r578060880
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/GenericJsonWriter.java
##########
@@ -36,7 +38,11 @@ public GenericJsonWriter() {
@Override
public byte[] write(GenericRecord message) {
try {
- return objectMapper.writeValueAsBytes(((GenericJsonRecord)message).getJsonNode().toString());
+ ByteArrayOutputStream buffer = new ByteArrayOutputStream();
Review comment:
+1 to what @MarvinCai said
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] MarvinCai commented on a change in pull request #9608: Fix writing/encoding of GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on a change in pull request #9608:
URL: https://github.com/apache/pulsar/pull/9608#discussion_r577746589
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/GenericJsonWriter.java
##########
@@ -36,7 +38,11 @@ public GenericJsonWriter() {
@Override
public byte[] write(GenericRecord message) {
try {
- return objectMapper.writeValueAsBytes(((GenericJsonRecord)message).getJsonNode().toString());
+ ByteArrayOutputStream buffer = new ByteArrayOutputStream();
Review comment:
I wonder if there's any specific reason we want to create our own ByteArrayOutputStream & JsonGenerator, which is what [objectMapper.writeValueAsBytes()](https://github.com/FasterXML/jackson-databind/blob/7ccdb596ce6ce3389aa5b9824cab70441fd72322/src/main/java/com/fasterxml/jackson/databind/ObjectMapper.java#L1699-L1708) 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[]).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #9608: Fix writing/encoding of GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #9608:
URL: https://github.com/apache/pulsar/pull/9608
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org