[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #1637: AVRO-3486: Set namespace with protocol fullName

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

KalleOlaviNiemitalo commented on code in PR #1637:
URL: https://github.com/apache/avro/pull/1637#discussion_r916532228


##########
lang/java/avro/src/main/java/org/apache/avro/Protocol.java:
##########
@@ -275,15 +274,29 @@ public Protocol(Protocol p) {
 
   public Protocol(String name, String doc, String namespace) {
     super(PROTOCOL_RESERVED);
-    this.name = name;
+    setName(name, namespace);
     this.doc = doc;
-    this.namespace = namespace;
   }
 
   public Protocol(String name, String namespace) {
     this(name, null, namespace);
   }
 
+  private void setName(String name, String namespace) {
+    int lastDot = name.lastIndexOf('.');
+    if (lastDot < 0) {
+      this.name = name;
+      this.namespace = namespace;
+    } else {

Review Comment:
   If a protocol has namespace "java" and name "lang.String", then its fullname should be "lang.String" for sure (not "java.lang.String"). But it is not clear from the spec whether types defined in this protocol should then inherit the "java" namespace (as declared) or the "lang" namespace (based on the fullname of the protocol). I don't see any justification for making nested types inherit the "java.lang" namespace.
   
   Relevant parts of the spec:
   
   <https://github.com/apache/avro/blob/4e1fefca493029ace961b7ef8889a3722458565a/doc/src/content/xdocs/spec.xml#L296-L316>
   <https://github.com/apache/avro/blob/4e1fefca493029ace961b7ef8889a3722458565a/doc/src/content/xdocs/spec.xml#L850-L851>
   
   In particular:
   
   > For example, if `"name": "X"` is specified, and this occurs within a field of the record definition of `org.foo.Y`, then the fullname is `org.foo.X`.
   
   If that is intended to apply only when there is a `"namespace":"org.foo"` property in the record definition or in some outer definition, and not when the record definition has `"name":"org.foo.Y", "namespace":"something_else"`, then the specification should say so.



-- 
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@avro.apache.org

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