matriv commented on pull request #18479:
URL: https://github.com/apache/flink/pull/18479#issuecomment-1021979678
> > 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.
Thx for explaining! I see your point. The decision was more towards JSON and
code simplification, so that this int id is part of the one liner `context`
field, so that everything is handled in one place regarding the complete
identification of a node. So we read the `context` as one POJO from JSON, and
we construct a new `context` from the annotation plus a freshly incremented id
for new nodes. Imho, I don't see a big issue in this approach, since we have
javadocs describing the usage of those sub-fields of the context, and I don't
see any problem for tooling, since it's a well defined string with 3
components, and a simple `split("_")` gives those independently.
@twalthr 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.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
