Add schema admin api get schema info with schema version #4877
Conversation
| @@ -461,11 +465,28 @@ public void getVersionBySchema( | |||
| } | |||
|
|
|||
| private static GetSchemaResponse convertSchemaAndMetadataToGetSchemaResponse(SchemaAndMetadata schemaAndMetadata) { | |||
| String schemaData; | |||
| if (schemaAndMetadata.schema.getType() == SchemaType.KEY_VALUE) { | |||
There was a problem hiding this comment.
Can you add test cases for this code path as well?
There was a problem hiding this comment.
Can you add test cases for this code path as well?
OK, I will add the tests
| @@ -461,11 +465,28 @@ public void getVersionBySchema( | |||
| } | |||
|
|
|||
| private static GetSchemaResponse convertSchemaAndMetadataToGetSchemaResponse(SchemaAndMetadata schemaAndMetadata) { | |||
| String schemaData; | |||
| if (schemaAndMetadata.schema.getType() == SchemaType.KEY_VALUE) { | |||
| ByteBuf byteBuf = ByteBufAllocator.DEFAULT.heapBuffer().writeBytes(schemaAndMetadata.schema.getData()); | |||
There was a problem hiding this comment.
I think KeyValueSchemaInfo or KeyValueSchema provides the until functions to split the key schema and value schema. We should try to use those util functions rather than reimplementing the logic here again.
sijie
added
area/admin
type/feature
|
run Integration Tests |
|
run cpp tests |
|
run java8 tests |
|
run Integration Tests |
| } | ||
|
|
||
| @Test(dataProvider = "schemas") | ||
| public void testSchemaInfoWithVersionApi(Schema<?> schema) throws Exception { |
There was a problem hiding this comment.
what is the difference with line 113?
| @@ -236,4 +245,36 @@ public String getTopicName() { | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
|
|
|||
| public static String getKeyValueSchemaString(SchemaData schemaData) { | |||
There was a problem hiding this comment.
Use KeyValueSchemaInfo.decodeKeyValueSchemaInfo:
- covert SchemaData to SchemaInfo
- KeyValue<SchemaInfo, SchemaInfo> kvsSchemaInfo = KeyValueSchemaInfo.decodeKeyValueSchemaInfo(schemaInfo);
- Use ObjectMapper to convert kvSchemaInfo into a json string.
The schemaInfo serializer and deserializer is already defined in SchemaUtils. You can modify it to handle KEY_VALUE correctly.
|
run cpp tests |
There was a problem hiding this comment.
@congbobo184 it seems that you made changes to support key/value schema info. but you didn't add tests for key/value schema info. Can you please add tests for it?
| @NoArgsConstructor | ||
| @Accessors(chain = true) | ||
| @Builder | ||
| public class KeyValueSchemaInfoData { |
There was a problem hiding this comment.
@congbobo any reason creating a new class? Can't you just KeyValue<Object, Object>?
There was a problem hiding this comment.
@congbobo any reason creating a new class? Can't you just
KeyValue<Object, Object>?
yes, there should use keyValue<Object, Object>, and then i will change it and add the test for it .
2.Delete keyValueSchemaInfoData then use KeyValue<Object, Object>
|
Change the Milestone to 2.4.2, because of conflict. |
Motivation
To fix #4854 and support get keyValueSchema
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (yes)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)
Documentation
Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation