Schema registry 4/N - #1381
Conversation
# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java # pulsar-common/src/main/java/org/apache/pulsar/common/api/Commands.java
merlimat
left a comment
merlimat
left a comment
There was a problem hiding this comment.
Change looks good, just a couple of questions on the removed fields from protobuf
| byte[] encode(T message); | ||
| T decode(byte[] bytes); | ||
|
|
||
| SchemaInfo getSchemaInfo(); |
There was a problem hiding this comment.
If SchemaInfo becomes part of API, we should move it in the same ..client.api package
There was a problem hiding this comment.
It's used in multiple places. Are you suggesting I keep it in common but add it to the client package?
| private State state; | ||
|
|
||
| private final ConcurrentLongHashMap<CompletableFuture<Pair<String, Long>>> pendingRequests = new ConcurrentLongHashMap<>( | ||
| private final ConcurrentLongHashMap<CompletableFuture<Triple<String, Long, byte[]>>> pendingRequests = new ConcurrentLongHashMap<>( |
There was a problem hiding this comment.
Maybe we could switch to a named class instead of the Triple so that it's easier to follow the meaning of each of them.
There was a problem hiding this comment.
Yeah, I had a similar thought but took the easy way out :-( I'll add a named class.
|
|
||
| message CommandSuccess { | ||
| required uint64 request_id = 1; | ||
| optional Schema schema = 2; |
There was a problem hiding this comment.
Wasn't this needed to get the consumer to be able to auto-discover the schema at the subscribe time?
| } | ||
|
|
||
| required string name = 1; | ||
| required bytes version = 2; |
There was a problem hiding this comment.
Shouldn't we need to know the version in some cases?
I'd imagine that a producer deriving the schema from a class type, won't know the version. But a consumer receiving a schema, might need the version?
# Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java # pulsar-common/src/main/java/org/apache/pulsar/common/api/Commands.java
|
@mgodave there's a file that's missing the license header: |
|
@mgodave please merge with master again and it should go through CI this time. |
|
@mgodave there is a conflict with a change that went into master again, sorry. I had changed |
merlimat
added
the
type/feature
|
retest this please |
#16026) Cherry-pick #15956. ### Motivation Currently, we need admin permissions to operate the schema API. This is because the admin permission was defined when the schema API was first added. See #1381. Later, then adding authentication granularity with #6428, we don't change the schema API part. So leave the admin permission today. But the binary protocol allows the produce/consume permission to get the schema, so change the related method permission to `produce/consume`.
…#15956) (apache#16026) Cherry-pick apache#15956. ### Motivation Currently, we need admin permissions to operate the schema API. This is because the admin permission was defined when the schema API was first added. See apache#1381. Later, then adding authentication granularity with apache#6428, we don't change the schema API part. So leave the admin permission today. But the binary protocol allows the produce/consume permission to get the schema, so change the related method permission to `produce/consume`. (cherry picked from commit f3b4e86)
3 participants
See #1137 for reference