[GitHub] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r171609211
##
File path: pulsar-common/src/main/proto/PulsarApi.proto
##
@@ -197,6 +205,8 @@ message CommandSubscribe {
repeated KeyValue metadata = 10;
optional bool read_compacted = 11;
+
+ optional int64 schema_version = 12;
Review comment:
I have changed this to pass the schema itself.
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r171608954
##
File path: pulsar-common/src/main/proto/PulsarApi.proto
##
@@ -22,6 +22,13 @@ 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;
Review comment:
re: type. I will acquiesce. I prefer the "open" implementation but recognize
that the current set of types is limited and growing slowly or not at all.
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r171608603
##
File path: pulsar-common/src/main/proto/PulsarApi.proto
##
@@ -22,6 +22,13 @@ 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;
Review comment:
Re: version, I have no way of telling what the backend will use to represent
a version. If we pigeon hole implementations into using a long then we could
force pulsar to store extra state to convert between a "pulsar version" and a
"storage version". Additionally, I reasoned that you either receive a version
string from an API, for example the REST endpoint, or through a consumed event,
like through the consumer. This data does not need to be human readable but we
can think of schemes to make it so (hash, encoded, etc).
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r171607212
##
File path: pulsar-common/src/main/proto/PulsarApi.proto
##
@@ -387,6 +400,7 @@ message CommandProducerSuccess {
// The last sequence id that was stored by this producer in the
previous session
// This will only be meaningful if deduplication has been enabled.
optional int64 last_sequence_id = 3 [default = -1];
+ optional Schema schema = 4;
Review comment:
done
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168294644
##
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. I had originally proposed that but it was eliminated based on some input
but I don't recall the specific circumstances.
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168283925
##
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:
@dave2wave I've been thinking about this more. If this is a common use case
I think I can make this happen with little upheaval. Let me know if this is a
deal breaker for you.
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168277614
##
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:
That is not impossible, but perhaps inconvenient. What is the use case here?
Are we talking within the same namespace or across namespaces? It would be
possible to add another layer of indirection to the schema and store the name
associated with the topic somewhere (not sure where at the moment).
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168275516
##
File path: pulsar-common/src/main/proto/PulsarApi.proto
##
@@ -79,8 +86,9 @@ message MessageMetadata {
repeated EncryptionKeys encryption_keys = 13;
// Algorithm used to encrypt data key
optional string encryption_algo = 14;
+ optional bytes schema_version = 15;
// Additional parameters required by encryption
- optional bytes encryption_param = 15;
+ optional bytes encryption_param = 16;
Review comment:
Sorry, I missed that.
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168275383
##
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:
The properties list is available to store that info. This makes the most
sense to me but I'm willing to change it if you are dead set on specifying this
information statically.
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168271879
##
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:
I tried to address this in a question posed on the original reference PR. I
had originally specced this out with that intent but after much thought I
decided not to add it. My intent was that the schema registry should just store
a "Schema" no matter what it is, it does not need to know what it contains. The
schema only really means something to the end systems (producer/consumer) and
by extension the broker. We can allow the broker to "plug-in" modules to allow
semantic parsing of the schema as needed. This way we can support any number of
crazy business cases.
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r168271025
##
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:
grrr, I HATE 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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r167996411
##
File path: pulsar-common/src/main/proto/PulsarApi.proto
##
@@ -272,6 +289,8 @@ message CommandProducer {
/// Add optional metadata key=value to this producer
repeated KeyValue metadata= 6;
+
+ optional int64 schema_version = 7;
Review comment:
This seems to be an oversight on my part. Since this series of changes is
concerned with a 'repository' I appear to have glossed over this point.
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r167996154
##
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 = 7;
+repeated KeyValue properties = 8;
+}
Review comment:
I took a comment from the PIP email thread regarding the arbitrariness of
the fields chosen in the original proposal. It occurred to me that schema type
is equally as arbitrary. First, adding a new type would require a binary
protocol change, a new cut release, and a coordinated client/server deployment.
By removing the schema as a hard coded field we're making the choice of how to
identify it an end-to-end problem. If, as you commented above, we need the
client to send the actual schema (which is an oversight on my part), then we
only need to compare the schemas. If we need to compare them semantically then
we can devise a server side plugin scheme to allow us to identify and compare
"like" versions ("is this avro schema 'compatible' with this other avro
schema?" for instance).
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] mgodave commented on a change in pull request #1232: Schema registry (1/4)
mgodave commented on a change in pull request #1232: Schema registry (1/4)
URL: https://github.com/apache/incubator-pulsar/pull/1232#discussion_r167994769
##
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 = 7;
Review comment:
Yeah definitely, I deleted a few fields and didn't readjust the numbering.
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
