[ISSUE #453] OpenSchema integration with EventMesh #434
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to the Apache EventMesh (incubating) community!!
This is your first PR in our project. We're very excited to have you onboard contributing. Your contributions are greatly appreciated!
Please make sure that the changes are covered by tests.
We will be here shortly.
Let us know if you need any help!
Want to get closer to the community?
WeChat Group：
Mailing Lists:
| Name | Description | Subscribe | Unsubscribe | Archive |
|---|---|---|---|---|
| Users | User support and questions mailing list | Subscribe | Unsubscribe | Mail Archives |
| Development | Development related discussions | Subscribe | Unsubscribe | Mail Archives |
| Commits | All commits to repositories | Subscribe | Unsubscribe | Mail Archives |
As shown above, the above APIs are not very closely following best practice of REST APIs. Therefore, I have proposed in the issue #339 to restify the EventMesh Schema Registry APIs by adding a new module. |
Codecov Report
@@ Coverage Diff @@
## develop #434 +/- ##
============================================
- Coverage 8.49% 8.38% -0.12%
Complexity 256 256
============================================
Files 228 231 +3
Lines 10783 10930 +147
Branches 918 932 +14
============================================
Hits 916 916
- Misses 9793 9940 +147
Partials 74 74
Continue to review full report at Codecov.
|
qqeasonchen
changed the title
|
@yzhao244 License check seems failed. |
|
@yzhao244 It might be better to add a new module such like And we have recently enhanced the SPI module #419, you can use |
yes, @ruanwenjun It seems also needs a eventmesh-connector-plugin moudle. |
eventmesh-runtime/build.gradle
Outdated
| @@ -29,8 +29,12 @@ List open_message = [ | |||
| "io.openmessaging:openmessaging-api:2.2.1-pubsub" | |||
| ] | |||
|
|
|||
| List h2_database = [ | |||
There was a problem hiding this comment.
This dependency should move to eventmesh-store-h2.
| @@ -57,6 +60,10 @@ public void start() throws IOException { | |||
| server.createContext("/clientManage/redirectClientByPath", new RedirectClientByPathHandler(eventMeshTCPServer)); | |||
| server.createContext("/clientManage/redirectClientByIpPort", new RedirectClientByIpPortHandler(eventMeshTCPServer)); | |||
| server.createContext("/clientManage/showListenClientByTopic", new ShowListenClientByTopicHandler(eventMeshTCPServer)); | |||
|
|
|||
| server.createContext("/schemaregistry/createSubject", new CreateSubjectHandler()); | |||
There was a problem hiding this comment.
schemaregistry should rename to schemaRegistry
| @@ -0,0 +1,94 @@ | |||
| package org.apache.eventmesh.runtime.admin.handler; | |||
There was a problem hiding this comment.
fixed license header
|
|
||
| private Logger logger = LoggerFactory.getLogger(CreateSubjectHandler.class); | ||
|
|
||
| private static ObjectMapper jsonMapper; |
There was a problem hiding this comment.
It might be better use JsonUtils to serialize or deserialize JSON.
There was a problem hiding this comment.
@ruanwenjun just to confirm :) .. do you mean by creating a new utility class called "JsonUtils" under path (org.apache.eventmesh.runtime.util) which leverages ObjectMapper for JSON serialization or deserialization?
There was a problem hiding this comment.
Maybe it can be placed at common module
| logger.error(result); | ||
| out.write(result.getBytes()); | ||
| return; | ||
| } finally { |
There was a problem hiding this comment.
Use try with resource instead of try finally
There was a problem hiding this comment.
In catch block, out.write(result.getBytes()); is used to return error response.. Therefore, out.write(result.getBytes()) becomes undefined if defining "OutputStream out" using Try with resource
|
|
||
| private String compatibilityLevel; | ||
|
|
||
| /* @ApiModelProperty(value = "Compatability Level", |
| /* @ApiModelProperty(value = "Compatability Level", | ||
| allowableValues = "BACKWARD, BACKWARD_TRANSITIVE, FORWARD, FORWARD_TRANSITIVE, FULL, " | ||
| + "FULL_TRANSITIVE, NONE")*/ | ||
| @JsonProperty("compatibility") |
There was a problem hiding this comment.
It might make confusion why the returned attribute don't match.
| private String version; | ||
|
|
||
| @JsonCreator | ||
| public SchemaCreateRequest(@JsonProperty("name") String name, |
There was a problem hiding this comment.
Remove @JsonCreator and @JsonProperty
|
|
||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
|
||
| public class BaseRequest { |
There was a problem hiding this comment.
The request might implement Serializable
|
|
||
| } | ||
|
|
||
| public static synchronized DBDataSource createDataSource(DBConfiguration dbConfig) { |
There was a problem hiding this comment.
The DBDataSource is singleton?
There was a problem hiding this comment.
Yes, DBDataSource is singleton. Basically, it performs init, start and shutdown as part of EventMeshServer init, start and shutdown process.
There was a problem hiding this comment.
But when we execute createDataSource method twice, we will get two DBDataSource instances, this is confusion, I think you would better make this class to be a real singleton.
There was a problem hiding this comment.
You are right. I pushed fixes to DBDataSource class. thx
|
@qqeasonchen You are right, the |
|
@qqeasonchen @ruanwenjun Thanks a lot for the review comments. I will start to address them :) |
|
Hello, @yzhao244 ! I looked interface at the development, it seems that some interfaces are missing. I refer to https://github.com/openmessaging/openschema/blob/master/spec.md |
…addressed other review comments
@Jackzeng1224 Thanks for the question. Yes, you are right. The current commit so far only contains three Subject related APIs such as create subject, readySubjectbyname and showAllSubjects which have been mentioned in my first comment of this PR. :) . However, I am going to implement the rest of missing interfaces as soon as we are okay with the new modules(eventmesh-store-api and eventmesh-store-plugin), because I can follow new modules standards to code/implement the rest of interfaces. lol |
@ruanwenjun Thanks for the suggestion. I have made eventmesh-store-h2 as a submodule of eventmesh-store-plugin module and enhanced my implementation by using EventMeshExtensionFactory.getExtension instead of ServiceLoader. :) |
Thanks for answering questions! Looking forward to eventmesh-store-plugin module |
|
@yzhao244 hello，I found a problem with createDataSource. First, the new H2SchemaAdapter is initialized. |
|
@yzhao244 hello,I looked at DBDataSource(), this "ds" does not belong to the object. Did you look right? |
You are right, I removed "static" when defining BasicDataSource ds. |
|
|
@qqeasonchen @ruanwenjun Hi guys, do you have any concerns before merging this PR? Please let me know if I missed anything here. :) |
@yzhao244 Hi, I have a question, the h2 is a memory database, it seems doesn't support distributed, how can the different eventmesh-runtime sync the schema change? |
|
|
@yzhao244 hello,this problem occurs with the new receiving code. "configurationWraper" or "configurationWrapper"? |
@qqeasonchen I answered your questions below. :)
|
very good question.. Actually, h2 can be turned into a persistent database by using the following example configuration (datasource.url=jdbc:h2:file:~/subdirectory/demodb) which persists data in file system. |
yzhao244
changed the title
@yzhao244 Individually separated PRs is good for me. But it is not very clear about how to use schema when pub/sub events, it seems eventmesh provides a management service for schema only in the request implementations, am i right? |
|
@yzhao244 Great, you mean if we use |
|
@yzhao244 hello，Can the module be tested and used normally now? or still to be modified. |
|
@yzhao244 hi, I suggest split this pr into several small ones, so that it can easily be reviewed and merged, for example, eventmesh-rest、eventmesh-store-api、eventmesh-store-h2 ect. |
|
@yzhao244 Could you please submmit this feature to branch openschema? i think eventmesh-rest can be merged to develop first. |
|
@qqeasonchen @Jackzeng1224 @ruanwenjun Hi guys, I agree with @qqeasonchen that splitting this pr into smaller ones will be easier and safer for reviewing and merging each individual plugin/module. Therefore, I am thinking to close this PR and will submit separate PRs for each module after some modifications. Additionally, @ruanwenjun brought a good point regarding decision-making of important design. Should I create a design doc and push it to https://github.com/apache/incubator-eventmesh/tree/develop/docs/en/features (similar as eventmesh-stream-design.md which explains design in details and gets the whole community's awareness instead of only explaining designs under an issue) :) |
|
@yzhao244 That would be good if the desing was clear. |
Fixes to child ISSUE#453 of its parent ISSUE#339.
Motivation
Explain why you want to make the changes and what problem you're trying to solve.
The Schema Registry is a central repository with RESTful interfaces for developers to define and register standard schemas. Addresses the problem of inconsistent or incompatible data(event) format between event producer and event consumer.
Modifications
Documentation