You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/07/18 09:32:29 UTC
[GitHub] [rocketmq-schema-registry] MatrixHB opened a new pull request, #4: optimize Schema Rest API and the cache refresh mechanism
MatrixHB opened a new pull request, #4:
URL: https://github.com/apache/rocketmq-schema-registry/pull/4
See the issue [ISSUE #2](https://github.com/apache/rocketmq-schema-registry/issues/2) 、 [ISSUE #3](https://github.com/apache/rocketmq-schema-registry/issues/3)
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-schema-registry] MatrixHB commented on a diff in pull request #4: optimize Schema Rest API and the cache refresh mechanism
Posted by GitBox <gi...@apache.org>.
MatrixHB commented on code in PR #4:
URL: https://github.com/apache/rocketmq-schema-registry/pull/4#discussion_r925352203
##########
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:
It's necessary to get schema by a given subject and a given version.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-schema-registry] ferrirW commented on a diff in pull request #4: optimize Schema Rest API and the cache refresh mechanism
Posted by GitBox <gi...@apache.org>.
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-schema-registry] ferrirW commented on a diff in pull request #4: optimize Schema Rest API and the cache refresh mechanism
Posted by GitBox <gi...@apache.org>.
ferrirW commented on code in PR #4:
URL: https://github.com/apache/rocketmq-schema-registry/pull/4#discussion_r925750116
##########
common/src/main/java/org/apache/rocketmq/schema/registry/common/storage/StorageServiceProxy.java:
##########
@@ -84,7 +86,7 @@ public void delete(final QualifiedName name) {
* @param schemaInfo schema information instance
* @return true if errors after this should be ignored.
*/
- @CacheEvict(key = "'schema.' + #name.getTenant() + '/' + #name.schema()")
Review Comment:
Does here also follow the design of tenant/schema?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-schema-registry] MatrixHB commented on a diff in pull request #4: optimize Schema Rest API and the cache refresh mechanism
Posted by GitBox <gi...@apache.org>.
MatrixHB commented on code in PR #4:
URL: https://github.com/apache/rocketmq-schema-registry/pull/4#discussion_r925355256
##########
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:
The rest apis are as follows now after I take your suggestion.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-schema-registry] MatrixHB commented on a diff in pull request #4: optimize Schema Rest API and the cache refresh mechanism
Posted by GitBox <gi...@apache.org>.
MatrixHB commented on code in PR #4:
URL: https://github.com/apache/rocketmq-schema-registry/pull/4#discussion_r924506697
##########
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:
just for test. I will delete it.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-schema-registry] duhenglucky merged pull request #4: optimize Schema Rest API and the cache refresh mechanism
Posted by GitBox <gi...@apache.org>.
duhenglucky merged PR #4:
URL: https://github.com/apache/rocketmq-schema-registry/pull/4
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-schema-registry] MatrixHB commented on a diff in pull request #4: optimize Schema Rest API and the cache refresh mechanism
Posted by GitBox <gi...@apache.org>.
MatrixHB commented on code in PR #4:
URL: https://github.com/apache/rocketmq-schema-registry/pull/4#discussion_r924508627
##########
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:
get it.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-schema-registry] MatrixHB commented on a diff in pull request #4: optimize Schema Rest API and the cache refresh mechanism
Posted by GitBox <gi...@apache.org>.
MatrixHB commented on code in PR #4:
URL: https://github.com/apache/rocketmq-schema-registry/pull/4#discussion_r926214774
##########
common/src/main/java/org/apache/rocketmq/schema/registry/common/storage/StorageServiceProxy.java:
##########
@@ -84,7 +86,7 @@ public void delete(final QualifiedName name) {
* @param schemaInfo schema information instance
* @return true if errors after this should be ignored.
*/
- @CacheEvict(key = "'schema.' + #name.getTenant() + '/' + #name.schema()")
Review Comment:
Get it.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org