Issue 227 add type in case it's not provided

[shshashwat]

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]

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.

[shrids]

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

[pbelgundi]

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.

[pbelgundi]

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

[shshashwat]

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

[pbelgundi]

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.

[pbelgundi]

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.

[pbelgundi]

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]

@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

[pbelgundi]

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

[shshashwat]

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.