[GitHub] [tez] mbathori-cloudera commented on a change in pull request #192: TEZ-4363: Bump protobuf dependency to 3.x

[GitBox <gi...@apache.org>]

mbathori-cloudera commented on a change in pull request #192:
URL: https://github.com/apache/tez/pull/192#discussion_r832045174



##########
File path: tez-dag/src/main/java/org/apache/tez/dag/history/events/AMLaunchedEvent.java
##########
@@ -90,7 +91,7 @@ public void toProtoStream(CodedOutputStream outputStream) throws IOException {
 
   @Override
   public void fromProtoStream(CodedInputStream inputStream) throws IOException {
-    AMLaunchedProto proto = inputStream.readMessage(AMLaunchedProto.PARSER, null);
+    AMLaunchedProto proto = inputStream.readMessage(AMLaunchedProto.parser(), ExtensionRegistry.newInstance());

Review comment:
       1. The PARSER field is deprecated in version 3 but replacing it with parser() is breaking backward compatibility so this needs to be reverted.
   2. Protobuf3 introduced a nullcheck on the extensionRegistry parameter so if it gets null value then it will throw a `NullpointerException`, that’s why I replaced it with `ExtensionRegistry.newInstance()` which is creating an empty instance (after checking this `getEmptyRegistry()` would be better since it is creating empty maps for the new instance)
   I checked the generated code both on Protobuf2 and Protobuf3 but it seems that `extensionRegistry` param is not used in our inputStream.readMessage use case.




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

To unsubscribe, e-mail: issues-unsubscribe@tez.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org