[GitHub] ivankelly commented on a change in pull request #1232: Schema registry (1/4)

2018-02-14 Thread GitBox
ivankelly commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168262139
 
 

 ##
 File path: pulsar-common/src/main/proto/PulsarApi.proto
 ##
 @@ -368,6 +387,7 @@ message CommandRedeliverUnacknowledgedMessages {
 
 message CommandSuccess {
required uint64 request_id = 1;
+   optional Schema schema = 2;
 
 Review comment:
   ok, in that case, this makes sense. perhaps it should be a repeated rather 
than an optional though, for the case that there's different versions.
   
   I guess that the schema name must be immutable for a topic then also.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services


[GitHub] ivankelly commented on a change in pull request #1232: Schema registry (1/4)

2018-02-14 Thread GitBox
ivankelly commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168240669
 
 

 ##
 File path: pulsar-common/src/main/proto/PulsarApi.proto
 ##
 @@ -368,6 +387,7 @@ message CommandRedeliverUnacknowledgedMessages {
 
 message CommandSuccess {
required uint64 request_id = 1;
+   optional Schema schema = 2;
 
 Review comment:
   Will the whole schema go with all messages? If that's the case, isn't this 
field here redundant?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services


[GitHub] ivankelly commented on a change in pull request #1232: Schema registry (1/4)

2018-02-14 Thread GitBox
ivankelly commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168196011
 
 

 ##
 File path: pulsar-common/src/main/proto/PulsarApi.proto
 ##
 @@ -22,6 +22,20 @@ package pulsar.proto;
 option java_package = "org.apache.pulsar.common.api.proto";
 option optimize_for = LITE_RUNTIME;
 
+message Schema {
+   required string name = 1;
 
 Review comment:
   tabs


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services


[GitHub] ivankelly commented on a change in pull request #1232: Schema registry (1/4)

2018-02-14 Thread GitBox
ivankelly commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168230410
 
 

 ##
 File path: pulsar-common/src/main/proto/PulsarApi.proto
 ##
 @@ -22,6 +22,20 @@ package pulsar.proto;
 option java_package = "org.apache.pulsar.common.api.proto";
 option optimize_for = LITE_RUNTIME;
 
+message Schema {
+   required string name = 1;
+   required bytes version = 2;
+   required bytes schema_data = 3;
 
 Review comment:
   Does the schema data itself have a schema? If we're supporting json, arvo, 
pb, etc, shouldn't we have a separate field to give the reader a hint of how 
they need to parse the schema?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services


[GitHub] ivankelly commented on a change in pull request #1232: Schema registry (1/4)

2018-02-14 Thread GitBox
ivankelly commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168229944
 
 

 ##
 File path: pulsar-common/src/main/proto/PulsarApi.proto
 ##
 @@ -22,6 +22,20 @@ package pulsar.proto;
 option java_package = "org.apache.pulsar.common.api.proto";
 option optimize_for = LITE_RUNTIME;
 
+message Schema {
+   required string name = 1;
+   required bytes version = 2;
+   required bytes schema_data = 3;
+repeated KeyValue properties = 4;
+}
+
+message Tombstone {}
+
+message SchemaEntry {
 
 Review comment:
   I assume this is part of a the schema registry part? Why is tombstone 
separate from the schema? You have schema_data as required? If tombstoning a 
schema (which I assume is deleting the schema) what should the data be? 
Couldn't schema data itself being empty denote the tombstone?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services


[GitHub] ivankelly commented on a change in pull request #1232: Schema registry (1/4)

2018-02-14 Thread GitBox
ivankelly commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168231228
 
 

 ##
 File path: pulsar-common/src/main/proto/PulsarApi.proto
 ##
 @@ -368,6 +387,7 @@ message CommandRedeliverUnacknowledgedMessages {
 
 message CommandSuccess {
required uint64 request_id = 1;
+   optional Schema schema = 2;
 
 Review comment:
   what happens if the message schema changes mid topic?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services