Issue 227 add type in case it's not provided #228

shshashwat · 2021-10-07T15:22:48Z

Signed-off-by: Shashwat Sharma [email protected]

Change log description
Currently if the type is not provided with addSchema call, for Protobuf and Avro it's being updated after parsing the file but in case of JSON and Custom it's not implemented and it throws retriable exception and keeps on retrying and ultimately after 1 minute the server returns with http status 504.

Purpose of the change
Fixes #227

What the code does
Implement similar behavior to Avro and Protobuf and update type if the same has not been provided with the API call addSchema

How to verify it
addSchema of JSON or Custom type without providing the argument "type" but the same should be reflected

shshashwat · 2021-10-09T01:15:08Z

Further update is that the update entries via the wirecommand fails if the "type" is not provided and this call is inside a retriable block and hence the code retries on fail and keeps on repeating the same unless the Server closes the connection with HTTP 504.
In case of AVRO and Protobuf there is a defined key which is used to update the "type" if the same has not been provided with the request call. Same logic can not applied to Custom and Json hence we will update the value with namespace and group.

server/src/main/java/io/pravega/schemaregistry/storage/client/TableStore.java

server/src/main/java/io/pravega/schemaregistry/storage/client/WireCommandClient.java

server/src/main/java/io/pravega/schemaregistry/storage/impl/schemas/PravegaKeyValueSchemas.java

server/src/main/java/io/pravega/schemaregistry/storage/impl/SchemaStoreImpl.java

server/src/main/java/io/pravega/schemaregistry/service/SchemaRegistryService.java

shrids

LGTM apart from a comment making the log lines more precise.

server/src/main/java/io/pravega/schemaregistry/storage/impl/schemas/PravegaKeyValueSchemas.java

pbelgundi · 2021-10-11T10:44:10Z

Further update is that the update entries via the wirecommand fails if the "type" is not provided and this call is inside a retriable block and hence the code retries on fail and keeps on repeating the same unless the Server closes the connection with HTTP 504. In case of AVRO and Protobuf there is a defined key which is used to update the "type" if the same has not been provided with the request call. Same logic can not applied to Custom and Json hence we will update the value with namespace and group.

Do we know why WireCommand.UpdateTableEntries fails when type is null or empty?
We noticed that it failed with a badkeyversion exception, which indicates the failure was due to inappropriate version being passed. How is this related to type feild being null/empty.

server/src/main/java/io/pravega/schemaregistry/service/SchemaRegistryService.java

pbelgundi · 2021-10-11T10:48:13Z

common/src/main/java/io/pravega/schemaregistry/common/NameUtil.java

+     * @param namespace the namespace for the schema
+     * @return Provided name or Created name for type in SchemaInfo
+     */
+    public static String createTypeIfAbsent(String type, String groupName, String namespace) {


I don;t see the need for this new method.

This removes the redundancy in the earlier code. Further, if need be this can be utilized in future too

IMO, this method is unnecessary. We could do with directly using NameUtil.qualifiedName(...);

kotlasaicharanreddy

Shashwat and me had tested this on a live cluster and didn't get any 504 error while creating a schema without type/name.

server/src/main/java/io/pravega/schemaregistry/service/SchemaRegistryService.java

server/src/test/java/io/pravega/schemaregistry/service/SchemaRegistryServiceTest.java

pbelgundi · 2021-10-11T12:23:56Z

server/src/main/java/io/pravega/schemaregistry/storage/impl/SchemaStoreImpl.java

@@ -133,7 +135,8 @@ public SchemaStoreImpl(Groups<T> groups, Schemas<T> schemas) {
    @Override
    public CompletableFuture<VersionInfo> addSchema(String namespace, String groupId, SchemaInfo schemaInfo, SchemaInfo normalized,
                                                    BigInteger fingerprint, GroupProperties prop, Etag etag) {
-        // Store normalized form of schema with the global schemas while the original form is stored within the group.  
+        // Store normalized form of schema with the global schemas while the original form is stored within the group.
+        log.info("Add schema called to add a new schema in namespace {} and groupId {}", namespace, groupId);


Here namespace passed by user may be an empty string, but we're adding the schema to default namespace in this case. So the log message should indicate that. Infact this is the best place to set the namespace string to its default value if the user has provided an empty String.

Not allowing empty String as value for any parameter can be enforced by specifying the minLength parameter in swagger .yaml file.
See: https://swagger.io/docs/specification/data-models/data-types/

pbelgundi · 2021-10-11T12:39:55Z

@shshashwat The real root cause here is that the SchemaInfo.type field that comes in from the user is empty (not null but empty) and while its fine to handle this in the backend a better thing to do would be to force the user to provide a value for this field using some swagger functionality.
Currently the schemaregistry.yaml lists the SchemaInfo.type field as required which basically just adds a NotNull check to the generated class. It may be better to do a Null + empty check and/or add more validations at this layer instead.
You can find the generated classes under io.pravega.schemaregistry.contract.data

common/src/main/java/io/pravega/schemaregistry/common/NameUtil.java

pbelgundi · 2021-10-12T05:02:35Z

common/src/main/java/io/pravega/schemaregistry/common/NameUtil.java

+     * @param namespace the namespace for the schema
+     * @return Provided name or Created name for type in SchemaInfo
+     */
+    public static String createTypeIfAbsent(String type, String groupName, String namespace) {


IMO, this method is unnecessary. We could do with directly using NameUtil.qualifiedName(...);

Signed-off-by: Shashwat Sharma <[email protected]>

shshashwat · 2021-10-12T11:47:47Z

The schemaInfo object is an autogenerated file. It takes all parameters from the request payload form UI or even the API. From UI we pass type which is an optional parameter but is mandatory for schemaInfo object and hence during the normalization of schema, we check if there is not a proper value assigned to it we assign the value to this field. The implementation is done earlier for AVRO and Protobuf but for JSON and Custom it was missing hence. Since the AVRO and Proto have a defined structure, we get the value from the file but JSON and Custom can take anything so proving a random UUID for the same.

common/src/main/java/io/pravega/schemaregistry/common/NameUtil.java

Signed-off-by: Shashwat Sharma <[email protected]>

shshashwat marked this pull request as ready for review October 7, 2021 15:40

shshashwat requested review from kotlasaicharanreddy, shrids and pbelgundi October 7, 2021 15:40

pbelgundi requested changes Oct 11, 2021

View reviewed changes

shrids reviewed Oct 11, 2021

View reviewed changes

server/src/main/java/io/pravega/schemaregistry/service/SchemaRegistryService.java Show resolved Hide resolved

shrids approved these changes Oct 11, 2021

View reviewed changes

server/src/main/java/io/pravega/schemaregistry/storage/impl/schemas/PravegaKeyValueSchemas.java Outdated Show resolved Hide resolved

pbelgundi requested changes Oct 11, 2021

View reviewed changes

kotlasaicharanreddy approved these changes Oct 11, 2021

View reviewed changes

pbelgundi requested changes Oct 11, 2021

View reviewed changes

server/src/main/java/io/pravega/schemaregistry/service/SchemaRegistryService.java Outdated Show resolved Hide resolved

server/src/test/java/io/pravega/schemaregistry/service/SchemaRegistryServiceTest.java Show resolved Hide resolved

pbelgundi reviewed Oct 11, 2021

View reviewed changes

pbelgundi requested changes Oct 12, 2021

View reviewed changes

shshashwat added 8 commits October 11, 2021 23:34

Issue 227 add type in case it's not provided

1605441

Signed-off-by: Shashwat Sharma <[email protected]>

Fix checkstyle

3a9bf3d

Signed-off-by: Shashwat Sharma <[email protected]>

Update type value using namespace and group

6d16ad9

Signed-off-by: Shashwat Sharma <[email protected]>

Add missing logs

66f4aa9

Signed-off-by: Shashwat Sharma <[email protected]>

Review comments

8a229c5

Signed-off-by: Shashwat Sharma <[email protected]>

Make a generic method call for createing type

39cfdb9

Signed-off-by: Shashwat Sharma <[email protected]>

Add condition if namespcae is null or empty

ddb8939

Signed-off-by: Shashwat Sharma <[email protected]>

Refactor the code and add javadoc

1c8021f

Signed-off-by: Shashwat Sharma <[email protected]>

shshashwat force-pushed the issue_227-Custom_Schema_error branch from db04b58 to 1c8021f Compare October 12, 2021 06:34

shshashwat added 2 commits October 11, 2021 23:53

Fix failing test case

6a9b3d4

Signed-off-by: Shashwat Sharma <[email protected]>

Add UUID to type

f57ad7b

Signed-off-by: Shashwat Sharma <[email protected]>

shrids reviewed Oct 12, 2021

View reviewed changes

common/src/main/java/io/pravega/schemaregistry/common/NameUtil.java Outdated Show resolved Hide resolved

REview comment

a1ba834

Signed-off-by: Shashwat Sharma <[email protected]>

shshashwat and others added 2 commits October 12, 2021 22:23

update default type value with all caps in test

1938f7f

Signed-off-by: Shashwat Sharma <[email protected]>

Merge branch 'master' into issue_227-Custom_Schema_error

9efd2c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 227 add type in case it's not provided #228

Issue 227 add type in case it's not provided #228

shshashwat commented Oct 7, 2021 •

edited

Loading

shshashwat commented Oct 9, 2021 •

edited

Loading

shrids left a comment

pbelgundi commented Oct 11, 2021

pbelgundi Oct 11, 2021

shshashwat Oct 11, 2021

pbelgundi Oct 12, 2021

kotlasaicharanreddy left a comment

pbelgundi Oct 11, 2021

pbelgundi Oct 11, 2021

pbelgundi commented Oct 11, 2021 •

edited

Loading

pbelgundi Oct 12, 2021

shshashwat commented Oct 12, 2021

Issue 227 add type in case it's not provided #228

Are you sure you want to change the base?

Issue 227 add type in case it's not provided #228

Conversation

shshashwat commented Oct 7, 2021 • edited Loading

shshashwat commented Oct 9, 2021 • edited Loading

shrids left a comment

Choose a reason for hiding this comment

pbelgundi commented Oct 11, 2021

pbelgundi Oct 11, 2021

Choose a reason for hiding this comment

shshashwat Oct 11, 2021

Choose a reason for hiding this comment

pbelgundi Oct 12, 2021

Choose a reason for hiding this comment

kotlasaicharanreddy left a comment

Choose a reason for hiding this comment

pbelgundi Oct 11, 2021

Choose a reason for hiding this comment

pbelgundi Oct 11, 2021

Choose a reason for hiding this comment

pbelgundi commented Oct 11, 2021 • edited Loading

pbelgundi Oct 12, 2021

Choose a reason for hiding this comment

shshashwat commented Oct 12, 2021

shshashwat commented Oct 7, 2021 •

edited

Loading

shshashwat commented Oct 9, 2021 •

edited

Loading

pbelgundi commented Oct 11, 2021 •

edited

Loading