You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2021/02/12 06:32:28 UTC

[GitHub] [samza] perkss opened a new pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

perkss opened a new pull request #1464:
URL: https://github.com/apache/samza/pull/1464


   Issues: The current version of Jackson is very old upgrade to latest version to get improvements, bug fixes and security enhancements. 
   Changes: Upgrade Jackson dependency to 2.12.1.
   Tests: Existing build tests
   API Changes: None
   Upgrade Instructions: None
   Usage Instructions: None


----------------------------------------------------------------
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] [samza] mynameborat commented on pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
mynameborat commented on pull request #1464:
URL: https://github.com/apache/samza/pull/1464#issuecomment-783549573


   I am out of office. @lakshmi-manasa-g can you take a look?


----------------------------------------------------------------
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] [samza] kw2542 commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
kw2542 commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r583039805



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
       Good point, I don't think we have to stick with SamzaException and it won't be necessary to wrap JsonProcessingException in SamzaException again. 




----------------------------------------------------------------
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] [samza] kw2542 commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
kw2542 commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r586053398



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
       Thank you for help driving the fix! Let's wait for their fix if it is not too long away.




----------------------------------------------------------------
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] [samza] perkss commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r582987562



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
        @kw2542 a question on the API here, for general use case for SamzaException when should we use this?  For example if we get a deserialization error generally or serialization error would we always want SamzaException to wrap the Jackson exception. Dont think this would happen now as they are allowed to throw JsonProcessingException. Therefore this exception is thrown inside a deserialize call so would follow the API more suitably if it was a Json exception. Or should we add a catch up and wrap any Jackson exception in a Samza Exception. 




----------------------------------------------------------------
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] [samza] perkss commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r583835787



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
       If we did want to achieve this to remain as was I raised an [issue](https://github.com/FasterXML/jackson-databind/issues/3068) in Jackson that stopped the [WRAP_EXCEPTION](https://fasterxml.github.io/jackson-databind/javadoc/2.12/com/fasterxml/jackson/databind/DeserializationFeature.html#WRAP_EXCEPTIONS) feature from working. With this disabled and tested against the SNAPSHOT it worked as originally. So we could wait for 2.12.2 to come out and it would behave the same.




----------------------------------------------------------------
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] [samza] perkss commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r581740149



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
       I do agree this seemed a bit icky, will try and work around it. Basically they are wrapping exceptions in Jackson as standard practice to add further context to the exception. The code that does this is nested here [MapDeserializer](https://github.com/FasterXML/jackson-databind/blob/jackson-databind-2.12.1/src/main/java/com/fasterxml/jackson/databind/deser/std/MapDeserializer.java#L621) which calls [ContainerDeserializerBase](https://github.com/FasterXML/jackson-databind/blob/jackson-databind-2.12.1/src/main/java/com/fasterxml/jackson/databind/deser/std/ContainerDeserializerBase.java#L173)
   
   
   The originator of this exception is here in Samza and states it is for 0.13 and 0.14 support is this throw still required if not will simplify this? [reference](https://github.com/apache/samza/blob/master/samza-core/src/main/java/org/apache/samza/serializers/model/SamzaObjectMapper.java#L109)
   
   Will continue to look into it.




----------------------------------------------------------------
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] [samza] perkss commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r583835787



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
       If we did want to achieve this to remain as was I raised an [issue](https://github.com/FasterXML/jackson-databind/issues/3068) in Jackson that stopped the [WRAP_EXCEPTION](https://fasterxml.github.io/jackson-databind/javadoc/2.12/com/fasterxml/jackson/databind/DeserializationFeature.html#WRAP_EXCEPTIONS) feature from working. They already fixed and merged. With this disabled and tested against the SNAPSHOT it worked as originally. So we could wait for 2.12.2 to come out and it would behave the same.




----------------------------------------------------------------
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] [samza] perkss commented on pull request #1464: SAMZA-2620: Jackson upgrade 2.12.2

Posted by GitBox <gi...@apache.org>.
perkss commented on pull request #1464:
URL: https://github.com/apache/samza/pull/1464#issuecomment-790425052


   @kw2542 the new version has been released, I have upgraded. So test back to how it was. Added in constraints on the Kafka module to manage the transitive Kafka dependencies of Jackson. Please review when you get a moment.


----------------------------------------------------------------
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] [samza] perkss edited a comment on pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss edited a comment on pull request #1464:
URL: https://github.com/apache/samza/pull/1464#issuecomment-778478778


   @mynameborat @nickpan47 @xinyuiscool @sborya do you think you could take a long at some point please? 


----------------------------------------------------------------
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] [samza] perkss commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r581740149



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
       I do agree this seemed a bit icky, will try and work around it. Basically they are wrapping exceptions in Jackson as standard practice to add further context to the exception. The code that does this is nested here [MapDeserializer](https://github.com/FasterXML/jackson-databind/blob/jackson-databind-2.12.1/src/main/java/com/fasterxml/jackson/databind/deser/std/MapDeserializer.java#L621) which calls [ContainerDeserializerBase](https://github.com/FasterXML/jackson-databind/blob/jackson-databind-2.12.1/src/main/java/com/fasterxml/jackson/databind/deser/std/ContainerDeserializerBase.java#L173)
   
   
   The originator of this exception is here in Samza and states it is for 0.13 and 0.14 support is this throw still required? [reference](https://github.com/apache/samza/blob/master/samza-core/src/main/java/org/apache/samza/serializers/model/SamzaObjectMapper.java#L109)
   
   Will continue to look into it.




----------------------------------------------------------------
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] [samza] perkss commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r583835787



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
       If we did want to achieve this I raised an [issue](https://github.com/FasterXML/jackson-databind/issues/3068) in Jackson that stopped the [WRAP_EXCEPTION](https://fasterxml.github.io/jackson-databind/javadoc/2.12/com/fasterxml/jackson/databind/DeserializationFeature.html#WRAP_EXCEPTIONS) feature from working. With this disabled and tested against the SNAPSHOT it worked as originally. So we could wait for 2.12.2 to come out and it would behave the same.




----------------------------------------------------------------
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] [samza] kw2542 merged pull request #1464: SAMZA-2620: Jackson upgrade 2.12.2

Posted by GitBox <gi...@apache.org>.
kw2542 merged pull request #1464:
URL: https://github.com/apache/samza/pull/1464


   


----------------------------------------------------------------
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] [samza] perkss edited a comment on pull request #1464: SAMZA-2620: Jackson upgrade 2.12.2

Posted by GitBox <gi...@apache.org>.
perkss edited a comment on pull request #1464:
URL: https://github.com/apache/samza/pull/1464#issuecomment-790425052


   @kw2542 the new version has been released, I have upgraded. So test back to how it was. Added in constraints on the Kafka module to manage the transitive Kafka dependencies of Jackson after that upgrade got merged. Please review when you get a moment.


----------------------------------------------------------------
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] [samza] perkss commented on pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss commented on pull request #1464:
URL: https://github.com/apache/samza/pull/1464#issuecomment-778478778


   @mynameborat @nickpan47 do you think you could take a long at some point please? 


----------------------------------------------------------------
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] [samza] perkss commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
perkss commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r582987562



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
        @kw2542 a question on the API here, for general use case for SamzaException when should we use this?  For example if we get a deserialization error generally or serialization error would we always want SamzaException to wrap the Jackson exception. Dont think this would happen now as they are allowed to throw JsonProcessingException. Therefore this exception is thrown inside a deserialize call so would follow the API more suitably if it was a Json exception. Or should we add a catch up and wrap any Jackson exception in a Samza Exception. 
   
   In the case above with the current default jackson handling it wraps the SamzaException with further details but the root cause is still SamzaException.




----------------------------------------------------------------
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] [samza] kw2542 commented on a change in pull request #1464: SAMZA-2620: Jackson upgrade 2.12.1

Posted by GitBox <gi...@apache.org>.
kw2542 commented on a change in pull request #1464:
URL: https://github.com/apache/samza/pull/1464#discussion_r581454368



##########
File path: samza-core/src/test/java/org/apache/samza/serializers/model/TestSamzaObjectMapper.java
##########
@@ -165,7 +165,7 @@ public void testDeserializeUnknownTaskModelField() throws IOException {
   /**
    * Given a {@link ContainerModel} JSON with neither a processor-id nor a container-id, deserialization should fail.
    */
-  @Test(expected = SamzaException.class)

Review comment:
       Can you help us understand why the exception type changed?

##########
File path: samza-core/src/main/java/org/apache/samza/clustermanager/container/placement/ContainerPlacementMessageObjectMapper.java
##########
@@ -23,21 +23,16 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.UUID;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.ObjectCodec;
+import com.fasterxml.jackson.core.Version;
+import com.fasterxml.jackson.databind.*;

Review comment:
       wildcard import is not allowed in non test sources.

##########
File path: samza-core/src/main/java/org/apache/samza/serializers/model/SamzaObjectMapper.java
##########
@@ -19,9 +19,13 @@
 
 package org.apache.samza.serializers.model;
 
-import java.io.IOException;
-import java.util.HashMap;
-import java.util.Map;
+import com.fasterxml.jackson.core.*;

Review comment:
       same




----------------------------------------------------------------
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