[GitHub] [flink] slinkydeveloper commented on pull request #18479: [FLINK-25387] Introduce ExecNodeMetadata

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

slinkydeveloper commented on pull request #18479:
URL: https://github.com/apache/flink/pull/18479#issuecomment-1021961660


   > Because, with the upgrade story, we can have an @ExecNodeMetadata annotation with the same name, on a subclass of a current ExecNode class, which does something new/different and defines a newer version. So we need the combination of name + version to uniquely identify the class when we lookup and rebuild the Java object graph from the JSON plan.
   
   I may have badly explained myself, but i'm not questioning the name + version tuple. That's ok and I get why we need it.
   
   What I'm questioning is that in the same field you add `id + name + version`, and this seems wrong to me, because `id` is the "instance identifier", like a pointer to a specific node of the graph, while `name + version` is the "type identifier", which tells you which `ExecNode` class and version the node is. I don't think these two concepts (instance id and type id) should be in the same field, as they are logically very different things, and also because future tooling will have hard time parsing this JSON. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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

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