[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=335649&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-335649
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 29/Oct/19 18:00
Start Date: 29/Oct/19 18:00
Worklog Time Spent: 10m 
  Work Description: TheMatrix97 commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-547554027
 
 
   @pabloem seems it's not published at maven central, but you can find it in 
the apache repo 
https://repository.apache.org/content/groups/snapshots/org/apache/beam/beam-sdks-java-core/2.18.0-SNAPSHOT/
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 335649)
Time Spent: 8.5h  (was: 8h 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 8.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=335647&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-335647
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 29/Oct/19 17:56
Start Date: 29/Oct/19 17:56
Worklog Time Spent: 10m 
  Work Description: pabloem commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-547552019
 
 
   ah every now and then the snapshot publishing seems to run into trouble. 
I'll ping the mailing list about 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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 335647)
Time Spent: 8h 20m  (was: 8h 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 8h 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=335628&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-335628
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 29/Oct/19 17:10
Start Date: 29/Oct/19 17:10
Worklog Time Spent: 10m 
  Work Description: TheMatrix97 commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-547531210
 
 
   @TheNeuralBit Actually I don't see the 2.18-SNAPSHOT version at Maven 
central repo :( The most recent one is 2.16.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 335628)
Time Spent: 8h 10m  (was: 8h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 8h 10m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=335627&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-335627
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 29/Oct/19 17:09
Start Date: 29/Oct/19 17:09
Worklog Time Spent: 10m 
  Work Description: mxm commented on issue #8496: [BEAM-5967] Add handling 
of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-547531113
 
 
   This doesn't appear to be in the `release-2.17.0` branch. So it will be part 
of Beam 2.18.0 release. Like Brian mentioned, you could still use the snapshot 
version. Or beg on the mailing list, that you want this to be in 2.17.0 ;)
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 335627)
Time Spent: 8h  (was: 7h 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 8h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=335624&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-335624
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 29/Oct/19 17:00
Start Date: 29/Oct/19 17:00
Worklog Time Spent: 10m 
  Work Description: TheNeuralBit commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-547526362
 
 
   Hey @TheMatrix97! I think this made it into the 2.17.0 release cut that 
happened last week. Usually it takes a few weeks to get a release ready after 
the cut. When it's finished there will be an announcement on [the mailing 
list](https://lists.apache.org/list.html?d...@beam.apache.org) and [the 
blog](https://beam.apache.org/blog/).
   
   Alternatively you should be able to get this feature if you use the 
2.18.0-SNAPSHOT version.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 335624)
Time Spent: 7h 50m  (was: 7h 40m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 7h 50m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=335609&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-335609
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 29/Oct/19 16:42
Start Date: 29/Oct/19 16:42
Worklog Time Spent: 10m 
  Work Description: TheMatrix97 commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-547517845
 
 
   Hi! Awesome work, thanks! When will this feature be available at maven 
central?
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 335609)
Time Spent: 7h 40m  (was: 7.5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 7h 40m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=334060&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-334060
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 25/Oct/19 10:13
Start Date: 25/Oct/19 10:13
Worklog Time Spent: 10m 
  Work Description: mxm commented on issue #8496: [BEAM-5967] Add handling 
of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-546293997
 
 
   Congrats @alexvanboxel! :)
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 334060)
Time Spent: 7.5h  (was: 7h 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 7.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=333690&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-333690
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 24/Oct/19 19:22
Start Date: 24/Oct/19 19:22
Worklog Time Spent: 10m 
  Work Description: pabloem commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-546065313
 
 
   Thanks everyone! Glad we were able to get this in : )
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 333690)
Time Spent: 7h 20m  (was: 7h 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 7h 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=333689&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-333689
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 24/Oct/19 19:22
Start Date: 24/Oct/19 19:22
Worklog Time Spent: 10m 
  Work Description: pabloem commented on pull request #8496: [BEAM-5967] 
Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496
 
 
   
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 333689)
Time Spent: 7h 10m  (was: 7h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 7h 10m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=333595&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-333595
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 24/Oct/19 17:06
Start Date: 24/Oct/19 17:06
Worklog Time Spent: 10m 
  Work Description: reuvenlax commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-546012761
 
 
   lgtm
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 333595)
Time Spent: 7h  (was: 6h 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 7h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=331855&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-331855
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 22/Oct/19 08:08
Start Date: 22/Oct/19 08:08
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r337376348
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   I split the DynamicProtoCoder from the ProtoCoder
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 331855)
Time Spent: 6h 50m  (was: 6h 40m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 6h 50m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=331854&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-331854
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 22/Oct/19 08:08
Start Date: 22/Oct/19 08:08
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r337376348
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   Split the DynamicProtoCoder from the ProtoCoder
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 331854)
Time Spent: 6h 40m  (was: 6.5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 6h 40m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=331580&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-331580
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 21/Oct/19 18:57
Start Date: 21/Oct/19 18:57
Worklog Time Spent: 10m 
  Work Description: reuvenlax commented on pull request #8496: [BEAM-5967] 
Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r337185284
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   Wondering the same thing. Might be cleaner to have a separate Coder for this 
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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 331580)
Time Spent: 6.5h  (was: 6h 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 6.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=331567&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-331567
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 21/Oct/19 18:17
Start Date: 21/Oct/19 18:17
Worklog Time Spent: 10m 
  Work Description: pabloem commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-544641391
 
 
   > Can this be merged before the 2.17 cut (scheduled for Wednesday, October 
23)?
   
   @reuvenlax 
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 331567)
Time Spent: 6h 20m  (was: 6h 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 6h 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=331541&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-331541
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 21/Oct/19 17:56
Start Date: 21/Oct/19 17:56
Worklog Time Spent: 10m 
  Work Description: TheNeuralBit commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-544632827
 
 
   Can this be merged before the 2.17 cut (scheduled for Wednesday, October 23)?
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 331541)
Time Spent: 6h 10m  (was: 6h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 6h 10m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=330330&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-330330
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 18/Oct/19 06:56
Start Date: 18/Oct/19 06:56
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-543552541
 
 
   > On the other hand, #8690 does test this more thoroughly, and it would be 
nice to get to remove these changes from that diff.
   
   Backposted the tests from the schema aware PR. If we get at least this PR in 
2.17 the schema aware one will be somewhat smaller.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 330330)
Time Spent: 6h  (was: 5h 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 6h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=330329&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-330329
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 18/Oct/19 06:55
Start Date: 18/Oct/19 06:55
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r336342831
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/test/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoderTest.java
 ##
 @@ -167,4 +170,26 @@ public void testNonDeterministicProperty() throws 
CoderException {
 Coder coder = ProtoCoder.of(MessageWithMap.class);
 assertNotEquals(CoderUtils.encodeToBase64(coder, msg2), 
CoderUtils.encodeToBase64(coder, msg1));
   }
+
+  @Test
+  public void testDynamicMessage() throws Exception {
 
 Review comment:
   Added another test `testDynamicNestedRepeatedMessage` that adds another 
repeated nested message.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 330329)
Time Spent: 5h 50m  (was: 5h 40m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 5h 50m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=330071&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-330071
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 17/Oct/19 19:00
Start Date: 17/Oct/19 19:00
Worklog Time Spent: 10m 
  Work Description: TheNeuralBit commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-543315949
 
 
   LGTM overall. I'm not an expert in proto descriptor wrangling either though, 
so some more testing (or confirmation from Reuven) would make me feel better. 
   
   On the other hand, #8690 does test this more thoroughly, and it would be 
nice to get to remove these changes from that diff.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 330071)
Time Spent: 5h 40m  (was: 5.5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 5h 40m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=330066&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-330066
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 17/Oct/19 18:54
Start Date: 17/Oct/19 18:54
Worklog Time Spent: 10m 
  Work Description: TheNeuralBit commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r336172573
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/test/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoderTest.java
 ##
 @@ -167,4 +170,26 @@ public void testNonDeterministicProperty() throws 
CoderException {
 Coder coder = ProtoCoder.of(MessageWithMap.class);
 assertNotEquals(CoderUtils.encodeToBase64(coder, msg2), 
CoderUtils.encodeToBase64(coder, msg1));
   }
+
+  @Test
+  public void testDynamicMessage() throws Exception {
 
 Review comment:
   Would it be too onerous to add a few more tests here? Maybe set some more 
fields and some nested fields?
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 330066)
Time Spent: 5.5h  (was: 5h 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 5.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=330060&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-330060
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 17/Oct/19 18:46
Start Date: 17/Oct/19 18:46
Worklog Time Spent: 10m 
  Work Description: TheNeuralBit commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r336168910
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   Could this be cleaner if you had `class DynamicMessageProtoCoder extends 
ProtoCoder`? Then you wouldn't need to branch based on `protoMessageClass == 
DynamicMessage` you just override the relevant functions.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 330060)
Time Spent: 5h 20m  (was: 5h 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 5h 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=328682&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-328682
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 15/Oct/19 17:09
Start Date: 15/Oct/19 17:09
Worklog Time Spent: 10m 
  Work Description: pabloem commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-542313697
 
 
   Thanks Alex! @reuvenlax @TheNeuralBit to review : )
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 328682)
Time Spent: 5h 10m  (was: 5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 5h 10m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=328378&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-328378
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 15/Oct/19 07:30
Start Date: 15/Oct/19 07:30
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r334790257
 
 

 ##
 File path: 
sdks/java/core/src/main/java/org/apache/beam/sdk/testing/CoderProperties.java
 ##
 @@ -104,6 +104,16 @@
 assertThat(decodeEncode(coder, context, value), equalTo(value));
   }
 
+  /**
+   * Verifies that for the given {@code Coder}, {@code Coder.Context}, and 
value of type {@code
+   * T}, encoding followed by decoding yields an equal value of type {@code T}.
 
 Review comment:
   Changed description
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 328378)
Time Spent: 5h  (was: 4h 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=328377&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-328377
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 15/Oct/19 07:29
Start Date: 15/Oct/19 07:29
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r334790174
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoDomain.java
 ##
 @@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.protobuf;
+
+import com.google.protobuf.DescriptorProtos;
+import com.google.protobuf.Descriptors;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+/**
+ * ProtoDomain is a container class for Protobuf descriptors. By using a 
domain for all descriptors
+ * that are related to each other the FileDescriptorSet needs to be serialized 
only once in the
+ * graph.
+ *
+ * Using a domain also grantees that all Descriptors have object equality, 
just like statically
+ * compiled Proto classes Descriptors. A lot of Java code isn't used to the 
new DynamicMessages an
+ * assume always Object equality. Because of this the domain class is 
immutable.
+ *
+ * ProtoDomains aren't assumed to be used on with normal Message 
descriptors, only with
+ * DynamicMessage descriptors.
+ */
+public final class ProtoDomain implements Serializable {
+  public static final long serialVersionUID = 1L;
+  private transient DescriptorProtos.FileDescriptorSet fileDescriptorSet;
+  private transient int hashCode;
+
+  private transient Map fileDescriptorMap;
+  private transient Map descriptorMap;
+
+  private transient Map fileOptionMap;
+  private transient Map messageOptionMap;
+  private transient Map fieldOptionMap;
+
+  ProtoDomain() {
+this(DescriptorProtos.FileDescriptorSet.newBuilder().build());
+  }
+
+  private ProtoDomain(DescriptorProtos.FileDescriptorSet fileDescriptorSet) {
+this.fileDescriptorSet = fileDescriptorSet;
+hashCode = java.util.Arrays.hashCode(this.fileDescriptorSet.toByteArray());
+crosswire();
+  }
+
+  private static Map 
extractProtoMap(
+  DescriptorProtos.FileDescriptorSet fileDescriptorSet) {
+HashMap map = new 
HashMap<>();
+fileDescriptorSet.getFileList().forEach(fdp -> map.put(fdp.getName(), 
fdp));
+return map;
+  }
+
+  private static Descriptors.FileDescriptor convertToFileDescriptorMap(
 
 Review comment:
   Added @Nullable (stange that github doesn't mark this file as Outdated)
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 328377)
Time Spent: 4h 50m  (was: 4h 40m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 4h 50m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and n

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=326483&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-326483
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 10/Oct/19 17:59
Start Date: 10/Oct/19 17:59
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-540701831
 
 
   I'll fix the comments tommorow (it's evening now)
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 326483)
Time Spent: 4h 40m  (was: 4.5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 4h 40m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=326451&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-326451
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 10/Oct/19 16:58
Start Date: 10/Oct/19 16:58
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-540678718
 
 
   The ProtoDomain is tested though the Protoaware support, I aligned the
   class with that PR, having this PR approved for this would make the schema
   aware PR smalled, as I will rebase the PR.
   
_/
   _/ Alex Van Boxel
   
   
   On Thu, Oct 10, 2019 at 6:55 PM Kenn Knowles 
   wrote:
   
   > *@kennknowles* approved this pull request.
   >
   > Sorry for the delay. This seems quite nice and useful. I am not expert in
   > proto descriptor wrangling. I would guess if there is a problem in that,
   > @reuvenlax  is more likely to catch it.
   >
   > Maybe follow up with more extensive testing exercising lots of cases of
   > ProtoDomain?
   > --
   >
   > In
   > 
sdks/java/core/src/main/java/org/apache/beam/sdk/testing/CoderProperties.java
   > :
   >
   > > @@ -104,6 +104,16 @@
   >  assertThat(decodeEncode(coder, context, value), equalTo(value));
   >}
   >
   > +  /**
   > +   * Verifies that for the given {@code Coder}, {@code Coder.Context}, 
and value of type {@code
   > +   * T}, encoding followed by decoding yields an equal value of type 
{@code T}.
   >
   > This one doesn't test that it is equal, but tests that the matcher
   > succeeds.
   > --
   >
   > In
   > 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoDomain.java
   > :
   >
   > > +  }
   > +
   > +  private ProtoDomain(DescriptorProtos.FileDescriptorSet 
fileDescriptorSet) {
   > +this.fileDescriptorSet = fileDescriptorSet;
   > +hashCode = 
java.util.Arrays.hashCode(this.fileDescriptorSet.toByteArray());
   > +crosswire();
   > +  }
   > +
   > +  private static Map 
extractProtoMap(
   > +  DescriptorProtos.FileDescriptorSet fileDescriptorSet) {
   > +HashMap map = new 
HashMap<>();
   > +fileDescriptorSet.getFileList().forEach(fdp -> map.put(fdp.getName(), 
fdp));
   > +return map;
   > +  }
   > +
   > +  private static Descriptors.FileDescriptor convertToFileDescriptorMap(
   >
   > @Nullable. I'm a bit sad our analyses didn't catch this.
   >
   > —
   > You are receiving this because you were mentioned.
   > Reply to this email directly, view it on GitHub
   > 
,
   > or unsubscribe
   > 

   > .
   >
   
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 326451)
Time Spent: 4.5h  (was: 4h 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 4.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAc

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=326449&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-326449
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 10/Oct/19 16:54
Start Date: 10/Oct/19 16:54
Worklog Time Spent: 10m 
  Work Description: kennknowles commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r333628729
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoDomain.java
 ##
 @@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.protobuf;
+
+import com.google.protobuf.DescriptorProtos;
+import com.google.protobuf.Descriptors;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+/**
+ * ProtoDomain is a container class for Protobuf descriptors. By using a 
domain for all descriptors
+ * that are related to each other the FileDescriptorSet needs to be serialized 
only once in the
+ * graph.
+ *
+ * Using a domain also grantees that all Descriptors have object equality, 
just like statically
+ * compiled Proto classes Descriptors. A lot of Java code isn't used to the 
new DynamicMessages an
+ * assume always Object equality. Because of this the domain class is 
immutable.
+ *
+ * ProtoDomains aren't assumed to be used on with normal Message 
descriptors, only with
+ * DynamicMessage descriptors.
+ */
+public final class ProtoDomain implements Serializable {
+  public static final long serialVersionUID = 1L;
+  private transient DescriptorProtos.FileDescriptorSet fileDescriptorSet;
+  private transient int hashCode;
+
+  private transient Map fileDescriptorMap;
+  private transient Map descriptorMap;
+
+  private transient Map fileOptionMap;
+  private transient Map messageOptionMap;
+  private transient Map fieldOptionMap;
+
+  ProtoDomain() {
+this(DescriptorProtos.FileDescriptorSet.newBuilder().build());
+  }
+
+  private ProtoDomain(DescriptorProtos.FileDescriptorSet fileDescriptorSet) {
+this.fileDescriptorSet = fileDescriptorSet;
+hashCode = java.util.Arrays.hashCode(this.fileDescriptorSet.toByteArray());
+crosswire();
+  }
+
+  private static Map 
extractProtoMap(
+  DescriptorProtos.FileDescriptorSet fileDescriptorSet) {
+HashMap map = new 
HashMap<>();
+fileDescriptorSet.getFileList().forEach(fdp -> map.put(fdp.getName(), 
fdp));
+return map;
+  }
+
+  private static Descriptors.FileDescriptor convertToFileDescriptorMap(
 
 Review comment:
   `@Nullable`. I'm a bit sad our analyses didn't catch this.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 326449)
Time Spent: 4h 10m  (was: 4h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 4h 10m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=326450&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-326450
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 10/Oct/19 16:54
Start Date: 10/Oct/19 16:54
Worklog Time Spent: 10m 
  Work Description: kennknowles commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r333626680
 
 

 ##
 File path: 
sdks/java/core/src/main/java/org/apache/beam/sdk/testing/CoderProperties.java
 ##
 @@ -104,6 +104,16 @@
 assertThat(decodeEncode(coder, context, value), equalTo(value));
   }
 
+  /**
+   * Verifies that for the given {@code Coder}, {@code Coder.Context}, and 
value of type {@code
+   * T}, encoding followed by decoding yields an equal value of type {@code T}.
 
 Review comment:
   This one doesn't test that it is equal, but tests that the matcher succeeds.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 326450)
Time Spent: 4h 20m  (was: 4h 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 4h 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=321449&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-321449
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 01/Oct/19 18:52
Start Date: 01/Oct/19 18:52
Worklog Time Spent: 10m 
  Work Description: pabloem commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-537177956
 
 
   r: @TheNeuralBit 
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 321449)
Time Spent: 4h  (was: 3h 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
> Fix For: 2.17.0
>
>  Time Spent: 4h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (Jira)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=304123&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-304123
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 30/Aug/19 09:03
Start Date: 30/Aug/19 09:03
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-526523756
 
 
   @reuvenlax @kennknowles , The ProtoDomain code from the Proto Schema support 
is now backported to this pull-request. I've created a pipeline on Dataflow 
with lots of static Protobufs and performed an upgrade for **2.14.0 -> 
2.16.0-SNAPSHOT** (build locally from this PR).
   
   What is `ProtoDomain` you will ask... well it's an **immutable wrapper** 
around the protobuf descriptor set that is not serializable. It takes over 
serialization with custom read/write object. It also provides indexes on 
everything on the set (the indexes are transient). If instantiating the 
ProtoCoder with a shared ProtoDomain object equality with the DynamicMessages 
is guaranteed.
   
   How is upgradability handled? ProtoCoder has now a fixed `serialVersionUID = 
-5043999806040629525L`, this is the UID from before the changes. The reference 
to ProtoDomain and the messageName are transient and handled by the custom 
read/write object methods. 
   
   It would be great if this still made the 2.16.0 release.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 304123)
Time Spent: 3h 50m  (was: 3h 40m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 3h 50m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.2#803003)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=272161&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-272161
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 04/Jul/19 11:46
Start Date: 04/Jul/19 11:46
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-508453493
 
 
   Well, as I'm sharing some code with the Protobuf Schema support, I like to
   keep this one on hold a bit, if thats ok.
   
_/
   _/ Alex Van Boxel
   
   
   On Thu, Jul 4, 2019 at 1:29 PM Maximilian Michels 
   wrote:
   
   > @kennknowles  @reuvenlax
   >  Should this go through another review
   > round?
   >
   > —
   > You are receiving this because you were mentioned.
   > Reply to this email directly, view it on GitHub
   > 
,
   > or mute the thread
   > 

   > .
   >
   
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 272161)
Time Spent: 3h 40m  (was: 3.5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 3h 40m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=272146&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-272146
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 04/Jul/19 11:29
Start Date: 04/Jul/19 11:29
Worklog Time Spent: 10m 
  Work Description: mxm commented on issue #8496: [BEAM-5967] Add handling 
of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-508448961
 
 
   @kennknowles @reuvenlax Should this go through another review round?
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 272146)
Time Spent: 3.5h  (was: 3h 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 3.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=248427&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-248427
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 25/May/19 15:12
Start Date: 25/May/19 15:12
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-495925622
 
 
   @kennknowles had to take some time for this. I set the `serialVersionUID` to 
the one before my changes, and also added a test to keep it locked. I also 
backtracked to see what could make the UID change and too my surprise the only 
thing that made it change was the addition of the `public static 
of(Descriptor)` method.
   
   I also rebased again master.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 248427)
Time Spent: 3h 20m  (was: 3h 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 3h 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=242744&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-242744
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 15/May/19 18:52
Start Date: 15/May/19 18:52
Worklog Time Spent: 10m 
  Work Description: kennknowles commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-492779218
 
 
   If you set `serialVersionUID` to exactly what it already is, then you keep 
compatibility nicely, no?
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 242744)
Time Spent: 3h 10m  (was: 3h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 3h 10m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=242303&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-242303
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 15/May/19 07:56
Start Date: 15/May/19 07:56
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-492548485
 
 
   Setting the serialVersionUID will break compatibility. I admit, my 
implementation is only forward compatible, although backward compatibility is 
not an issue because I only add bytes to the stream when it's a DynamicMessage.
   
   Anyway, I'm have the impression this will never get accepted this way: I'll 
propose I override the ProtoCoder (calling it DynamicMessageCoder) and do the 
special logic there only for DynamicMessage. WDYT?
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 242303)
Time Spent: 3h  (was: 2h 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 3h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=241380&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-241380
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 13/May/19 22:46
Start Date: 13/May/19 22:46
Worklog Time Spent: 10m 
  Work Description: kennknowles commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-492014074
 
 
   You should also set serialVersionUID to a fixed value.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 241380)
Time Spent: 2h 50m  (was: 2h 40m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 2h 50m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240901&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240901
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 13/May/19 06:36
Start Date: 13/May/19 06:36
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-491693602
 
 
   @reuvenlax @kennknowles if upgradeability is a problem I think I handled 
this correctly, please note this extract from the serialization spec:
   
   > 5.6.2 Compatible Changes
   > Adding writeObject/readObject methods - If the version reading the stream 
has these methods then readObject is expected, as usual, to read the required 
data written to the stream by the default serialization. It should call 
defaultReadObject first before reading any optional data. The writeObject 
method is expected as usual to call defaultWriteObject to write the required 
data and then may write optional data.
   
   ref: 
https://docs.oracle.com/javase/8/docs/platform/serialization/spec/version.html
   
   This is why:
   
   1. on line 285 the descriptor is added `transient`
   2. on line 303 the `writeObject` (and `readObject`) as added as in the spec 
(compatible)
   3. on line 304 the `oos.defaultWriteObject();` is added as in the spec 
(compatible)
   4. on line 305 writing the `Descriptor` **only** kicks in when the Class is 
`DynamicMessage`. Note that we will have no one that will have a pipeline 
running with `DynamicMessage`. This is because `DynamicMessage` doesn't have a 
`getDescriptor()` without arguments. (previous version of the Coder assumed 
this).
   
   
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240901)
Time Spent: 2h 40m  (was: 2.5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 2h 40m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240900&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240900
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 13/May/19 06:34
Start Date: 13/May/19 06:34
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-491693602
 
 
   @reuvenlax @kennknowles if upgradeability is a problem I think I handled 
this correctly, please note this extract from the serialization spec:
   
   > 5.6.2 Compatible Changes
   > Adding writeObject/readObject methods - If the version reading the stream 
has these methods then readObject is expected, as usual, to read the required 
data written to the stream by the default serialization. It should call 
defaultReadObject first before reading any optional data. The writeObject 
method is expected as usual to call defaultWriteObject to write the required 
data and then may write optional data.
   
   ref: 
https://docs.oracle.com/javase/8/docs/platform/serialization/spec/version.html
   
   This is why:
   
   1. on line 285 the descriptor is added `transient`
   2. on line 303 the `writeObject` (and `readObject`) as added as in the spec 
(compatible)
   3. on line 304 the `oos.defaultWriteObject();` is added as in the spec 
(compatible)
   4. on line 305 writing the `Descriptor` **only** kicks in when the Class is 
`DynamicMessage`. Note that we will have no one that will have a pipeline 
running with `DynamicMessage`. This is because `DynamicMessage` doesn't have a 
`getDescriptor()` without arguments.
   
   
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240900)
Time Spent: 2.5h  (was: 2h 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 2.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240801&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240801
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 12/May/19 21:29
Start Date: 12/May/19 21:29
Worklog Time Spent: 10m 
  Work Description: reuvenlax commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-491630816
 
 
   Alex, Kenn's concern is that people who make no changes to their pipeline
   (other than updating the Beam version) will be unable to update.
   ProtoSchemaCoder will not have this problem as people will need to make
   code changes to use it.
   
   *From: *Alex Van Boxel 
   *Date: *Sun, May 12, 2019 at 10:11 AM
   *To: *apache/beam
   *Cc: *reuvenlax, Mention
   
   *@alexvanboxel* commented on this pull request.
   > --
   >
   > In
   > 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
   > :
   >
   > >/** Private constructor. */
   >private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
   >  this.protoMessageClass = protoMessageClass;
   >  this.extensionHostClasses = extensionHostClasses;
   > +this.protoMessageDescriptor = null;
   > +  }
   > +
   > +  private ProtoCoder(
   > +  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
   > +@SuppressWarnings("unchecked")
   > +Class protoMessageClass = (Class) DynamicMessage.class;
   > +this.protoMessageClass = protoMessageClass;
   > +this.extensionHostClasses = extensionHostClasses;
   > +this.protoMessageDescriptor = protoMessageDescriptor;
   > +  }
   > +
   > +  private void writeObject(ObjectOutputStream oos) throws IOException {
   >
   > Well, | don't see another way todo this. I need to get the Descriptor to
   > the workers. The only way I see it working is sending the FileDescriptorSet
   > as a proto steam onto the Java serialized object stream.
   >
   > Getting everyone on ProtoSchemaProvider will have the exact same problem
   > I'm afraid. I'll know in a few days when I got a working prototype.
   >
   > —
   > You are receiving this because you were mentioned.
   > Reply to this email directly, view it on GitHub
   > , or mute
   > the thread
   > 

   > .
   >
   
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240801)
Time Spent: 2h 20m  (was: 2h 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 2h 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240782&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240782
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 12/May/19 18:18
Start Date: 12/May/19 18:18
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283145727
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   Well, | don't see another way todo this. I need to get the Descriptor to the 
workers. The only way I see it working is sending the FileDescriptorSet as a 
proto steam onto the Java serialized object stream.
   
   Getting everyone on ProtoSchemaProvider will have the exact same problem I'm 
afraid and I even think it will be worse as there is no way of sharing payload 
between the toRow and fromRow functions. I'll know in a few days when I got a 
working prototype.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240782)
Time Spent: 2h 10m  (was: 2h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 2h 10m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240776&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240776
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 12/May/19 17:11
Start Date: 12/May/19 17:11
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283145727
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   Well, | don't see another way todo this. I need to get the Descriptor to the 
workers. The only way I see it working is sending the FileDescriptorSet as a 
proto steam onto the Java serialized object stream.
   
   Getting everyone on ProtoSchemaProvider will have the exact same problem I'm 
afraid. I'll know in a few days when I got a working prototype.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240776)
Time Spent: 2h  (was: 1h 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 2h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240764&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240764
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 12/May/19 15:04
Start Date: 12/May/19 15:04
Worklog Time Spent: 10m 
  Work Description: reuvenlax commented on pull request #8496: [BEAM-5967] 
Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283140901
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   Oof, that's pretty bad. Maybe we should change that?
   
   An alternative would be to just put the DynamicMessage support in a 
ProtoSchemaProvider, and encourage people to stop using ProtoCoder altogether. 
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240764)
Time Spent: 1h 50m  (was: 1h 40m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 1h 50m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240737&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240737
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 12/May/19 08:31
Start Date: 12/May/19 08:31
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-491576105
 
 
   Force pushed to resolve weak type of `of(Descriptors.Descriptor`, also added 
missing **comment**.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240737)
Time Spent: 1h 40m  (was: 1.5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 1h 40m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240736&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240736
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 12/May/19 08:16
Start Date: 12/May/19 08:16
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283125184
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/test/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoderTest.java
 ##
 @@ -167,4 +169,20 @@ public void testNonDeterministicProperty() throws 
CoderException {
 Coder coder = ProtoCoder.of(MessageWithMap.class);
 assertNotEquals(CoderUtils.encodeToBase64(coder, msg2), 
CoderUtils.encodeToBase64(coder, msg1));
   }
+
+  @Test
+  public void testDynamicMessage() throws Exception {
+DynamicMessage message =
+DynamicMessage.newBuilder(MessageA.getDescriptor())
+.setField(
+
MessageA.getDescriptor().findFieldByNumber(MessageA.FIELD1_FIELD_NUMBER), "foo")
+.build();
+Coder coder = 
ProtoCoder.of(message.getDescriptorForType());
 
 Review comment:
   It can't. I'll change the `of(DynamicMessage` to return 
Coder as you suggested.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240736)
Time Spent: 1.5h  (was: 1h 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 1.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240734&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240734
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 12/May/19 08:14
Start Date: 12/May/19 08:14
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283125121
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   I think I pulled the upgradability of by using (and making protoMessageClass 
transient):
   ```
   oos.defaultWriteObject();
   if (DynamicMessage.class.equals(this.protoMessageClass)) {
   ```
   I don't know if a StructuredCoder would help because a Descriptor is quite a 
complex beast (I serialize the complete proto binary on the stream.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240734)
Time Spent: 1h 20m  (was: 1h 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 1h 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240733&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240733
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 12/May/19 08:08
Start Date: 12/May/19 08:08
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283124887
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -122,6 +126,11 @@
 return of(protoMessageClass);
   }
 
+  public static  ProtoCoder of(
 
 Review comment:
   Good point... it's indeed a DynamicMessage. Will change.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240733)
Time Spent: 1h 10m  (was: 1h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 1h 10m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240630&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240630
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 11/May/19 17:10
Start Date: 11/May/19 17:10
Worklog Time Spent: 10m 
  Work Description: kennknowles commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283104231
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/test/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoderTest.java
 ##
 @@ -167,4 +169,20 @@ public void testNonDeterministicProperty() throws 
CoderException {
 Coder coder = ProtoCoder.of(MessageWithMap.class);
 assertNotEquals(CoderUtils.encodeToBase64(coder, msg2), 
CoderUtils.encodeToBase64(coder, msg1));
   }
+
+  @Test
+  public void testDynamicMessage() throws Exception {
+DynamicMessage message =
+DynamicMessage.newBuilder(MessageA.getDescriptor())
+.setField(
+
MessageA.getDescriptor().findFieldByNumber(MessageA.FIELD1_FIELD_NUMBER), "foo")
+.build();
+Coder coder = 
ProtoCoder.of(message.getDescriptorForType());
 
 Review comment:
   This is the type I would expect `ProtoCoder.of(descriptor)`. If it really 
has some type magic where it can be `Coder` instead of 
`Coder` then I suggest a test of that functionality.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240630)
Time Spent: 1h  (was: 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 1h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240629&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240629
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 11/May/19 17:10
Start Date: 11/May/19 17:10
Worklog Time Spent: 10m 
  Work Description: kennknowles commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283104093
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -269,21 +278,61 @@ public ExtensionRegistry getExtensionRegistry() {
   private transient ExtensionRegistry memoizedExtensionRegistry;
   private transient Parser memoizedParser;
 
+  // Descriptor used by DynamicMessage.
+  private transient Descriptors.Descriptor protoMessageDescriptor;
+
   /** Private constructor. */
   private ProtoCoder(Class protoMessageClass, Set> 
extensionHostClasses) {
 this.protoMessageClass = protoMessageClass;
 this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = null;
+  }
+
+  private ProtoCoder(
+  Descriptors.Descriptor protoMessageDescriptor, Set> 
extensionHostClasses) {
+@SuppressWarnings("unchecked")
+Class protoMessageClass = (Class) DynamicMessage.class;
+this.protoMessageClass = protoMessageClass;
+this.extensionHostClasses = extensionHostClasses;
+this.protoMessageDescriptor = protoMessageDescriptor;
+  }
+
+  private void writeObject(ObjectOutputStream oos) throws IOException {
 
 Review comment:
   Hmm, interesting. It is a problem that ProtoCoder is a CustomCoder 
serialized via Java serialization rather than a StructuredCoder. Not related to 
your change, just noting. This essentially makes it incompatible with pipeline 
update. @reuvenlax 
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240629)
Time Spent: 1h  (was: 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 1h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240631&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240631
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 11/May/19 17:10
Start Date: 11/May/19 17:10
Worklog Time Spent: 10m 
  Work Description: kennknowles commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#discussion_r283104195
 
 

 ##
 File path: 
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoder.java
 ##
 @@ -122,6 +126,11 @@
 return of(protoMessageClass);
   }
 
+  public static  ProtoCoder of(
 
 Review comment:
   I'm not very familiar with DynamicMessage usage, but is it actually a `T`? 
Or is this a `Coder`?
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240631)
Time Spent: 1h  (was: 50m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 1h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=240604&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-240604
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 11/May/19 15:03
Start Date: 11/May/19 15:03
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-491518462
 
 
   Rebased against master to keep PR uptodate
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 240604)
Time Spent: 50m  (was: 40m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 50m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=239887&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-239887
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 09/May/19 18:28
Start Date: 09/May/19 18:28
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-491015211
 
 
   Added extra check to make sure that when a DynamicMessage is used a 
Descriptor is provided.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 239887)
Time Spent: 40m  (was: 0.5h)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 40m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=239128&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-239128
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 08/May/19 10:16
Start Date: 08/May/19 10:16
Worklog Time Spent: 10m 
  Work Description: mxm commented on issue #8496: [BEAM-5967] Add handling 
of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-490431394
 
 
   Thanks @alexvanboxel. I'll defer to Kenn and Reuven for now.
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 239128)
Time Spent: 0.5h  (was: 20m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=238683&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-238683
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 07/May/19 17:45
Start Date: 07/May/19 17:45
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on issue #8496: [BEAM-5967] Add 
handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496#issuecomment-490180416
 
 
   @mxm @reuvenlax @kennknowles This is the first draft of supporting ProtoBuf 
DynamicMessages. I took a design decision here (see commit) and like your 
feedback.
   
   ref: 
https://issues.apache.org/jira/browse/BEAM-5967?filter=-1&jql=project%20%3D%20BEAM%20AND%20assignee%20in%20(currentUser())%20order%20by%20updated%20DESC
 

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:
us...@infra.apache.org


Issue Time Tracking
---

Worklog Id: (was: 238683)
Time Spent: 20m  (was: 10m)

> ProtoCoder doesn't support DynamicMessage
> -
>
> Key: BEAM-5967
> URL: https://issues.apache.org/jira/browse/BEAM-5967
> Project: Beam
>  Issue Type: Improvement
>  Components: sdk-java-core
>Affects Versions: 2.8.0
>Reporter: Alex Van Boxel
>Assignee: Alex Van Boxel
>Priority: Major
>  Time Spent: 20m
>  Remaining Estimate: 0h
>
> The ProtoCoder does make some assumptions about static messages being 
> available. The DynamicMessage doesn't have some of them, mainly because the 
> proto schema is defined at runtime and not at compile time.
> Does it make sense to make a special coder for DynamicMessage or build it 
> into the normal ProtoCoder.
> Here is an example of the assumtion being made in the current Codec:
> {code:java}
> try {
>   @SuppressWarnings("unchecked")
>   T protoMessageInstance = (T) 
> protoMessageClass.getMethod("getDefaultInstance").invoke(null);
>   @SuppressWarnings("unchecked")
>   Parser tParser = (Parser) protoMessageInstance.getParserForType();
>   memoizedParser = tParser;
> } catch (IllegalAccessException | InvocationTargetException | 
> NoSuchMethodException e) {
>   throw new IllegalArgumentException(e);
> }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Work logged] (BEAM-5967) ProtoCoder doesn't support DynamicMessage

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BEAM-5967?focusedWorklogId=237230&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-237230
 ]

ASF GitHub Bot logged work on BEAM-5967:


Author: ASF GitHub Bot
Created on: 04/May/19 18:09
Start Date: 04/May/19 18:09
Worklog Time Spent: 10m 
  Work Description: alexvanboxel commented on pull request #8496: 
[BEAM-5967] Add handling of DynamicMessage in ProtoCoder
URL: https://github.com/apache/beam/pull/8496
 
 
   The ProtoCoder was unable to handle DynamicMessage as it was unable to
   get a message specific parser. The Coder is expanded to handle
   DynamicMessage as a special case. It stores the complete descriptor set
   when (de)serializing.
   
   Design decision: Although DynamicMessage could in theory have a
   different schema per message in a single stream this doesn't make sense.
   The common use-case for using DynamicMessages is when the schema is not
   known at compile type, but is known at pipeline build time (example,
   like pulled from a schema registry).
   
   With this restriction we only need to store the schema (or descriptor)
   once when the pipeline is serialized and send to the workers.
   
   **Please** add a meaningful description for your change here
   
   
   
   Thank you for your contribution! Follow this checklist to help us 
incorporate your contribution quickly and easily:
   
- [ ] [**Choose 
reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and 
mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in 
ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA 
issue, if applicable. This will automatically link the pull request to the 
issue.
- [ ] If this contribution is large, please file an Apache [Individual 
Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   Post-Commit Tests Status (on master branch)
   

   
   Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
   --- | --- | --- | --- | --- | --- | --- | ---
   Go | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/)
 | --- | --- | --- | --- | --- | ---
   Java | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/)
 | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/)
 | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)
 | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)[![Build
 
Status](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)[![Build
 
Status](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/)
 | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/)
 | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/)
 | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)
   Python | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Python_Verify/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python_Verify/lastCompletedBuild/)[![Build
 
Status](https://builds.apache.org/job/beam_PostCommit_Python3_Verify/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python3_Verify/lastCompletedBuild/)
 | --- | [![Build 
Status](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon)](https://builds.apac