ferrirW commented on code in PR #4:
URL:
https://github.com/apache/rocketmq-schema-registry/pull/4#discussion_r924442900
##########
common/src/main/java/org/apache/rocketmq/schema/registry/common/QualifiedName.java:
##########
@@ -34,10 +34,15 @@
public class QualifiedName implements Serializable {
private static final long serialVersionUID = 2266514833942841209L;
+ public static final String DEFAULT_TENANT = "default";
+
+ public static final String DEFAULT_CLUSTER = "cluster";
+
private String cluster;
private String tenant;
private String subject;
private String schema;
+ private Long version;
Review Comment:
I'm not sure if it's necessary to introduce version to here.
What's the main consideration here?
##########
core/src/main/java/org/apache/rocketmq/schema/registry/core/api/v1/SchemaController.java:
##########
@@ -60,13 +63,15 @@ public SchemaController(
final RequestProcessor requestProcessor,
final SchemaService<SchemaDto> schemaService
) {
+ this.cluster = QualifiedName.DEFAULT_CLUSTER;
+ this.tenant = QualifiedName.DEFAULT_TENANT;
this.requestProcessor = requestProcessor;
this.schemaService = schemaService;
}
@RequestMapping(
method = RequestMethod.POST,
- path =
"/cluster/{cluster-name}/subject/{subject-name}/schema/{schema-name}",
Review Comment:
Cluster/Tenant can be default, but i think there still needs interface to
expose to users. Otherwise we can only add clusters and tenants through
configuration and restart.
Just like the bottom two interfaces both need exist:
/cluster/{cluster-name}/subject/{subject-name}/schema/{schema-name}
/subject/{subject-name}/schema/{schema-name}
##########
common/src/main/java/org/apache/rocketmq/schema/registry/common/storage/StorageServiceProxy.java:
##########
@@ -110,12 +112,21 @@ public SchemaInfo get(final QualifiedName name, final
boolean useCache) {
return storageService.get(storageServiceContext, name);
}
- @Cacheable(key = "'subject.' + #subject", condition = "#useCache")
+ @Cacheable(key = "'subject.' + #name.getSubject() + '/' +
#name.getVersion()", condition = "#useCache && #name.getVersion() != null")
Review Comment:
I think don't have to add a version number here, but GetBySchemaName
requires an interface with version？
##########
common/src/main/java/org/apache/rocketmq/schema/registry/common/model/SchemaRecordInfo.java:
##########
@@ -37,6 +37,7 @@ public class SchemaRecordInfo implements Serializable {
private String idl;
private Dependency dependency;
private List<SubjectInfo> subjects;
+ private String type;
Review Comment:
You can transfer String to SchemaType.
##########
schema-storage-rocketmq/src/main/resources/rocketmq.properties:
##########
@@ -16,4 +16,4 @@
#
storage.type=rocketmq
-#storage.local.cache.path
\ No newline at end of file
+storage.local.cache.path=/Users/xyb/app/schema-registry/cache
Review Comment:
This value should be deleted
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
