[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign neuromage
You can assign the PR to them by writing /assign @neuromage in a comment when ready.

The full list of commands accepted by this bot can be found here.

Hi @dreamryx. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

@dreamryx why do you think this PR is needed? Is there a specific bug to be fixed?

The schema registration package is for predefined types and it has unit tests to verify the schemas actually load without errors such as stack overflow.

@zhenghuiwang Thanks for your review. :)

It's really a QA test case here:

1. Assume there are three schema a, b and c.
2. Assume a has parent ref b, b has parent ref c and c has parent ref a.
3. When load pre-defined schema, our server may go over like this "a -> b -> c -> a ...."

Yes. As you said, currently the pre-defined schema are well designed and tested.

But we need to add a guard to eliminate the concern in server:

1. User can add schemas in dockerfile to create new image that contains new pre-defined schema.
2. Maybe we could allow user to load their pre-defined schema in the future.

This PR could help improve the server. Hope you can approve it.

Thanks.

@dreamryx I agree there can be theoretical circular reference problem for the schemas. Is there any specific example of schema that you want to include that has circular reference?

IMHO we should not have circular reference at the first place and we should let the server panic (due to overflow) during start if there are. Circular reference is not just hard to write code to parse but also hard for human to consume. We should strive to make the schemas self-contain.

@zhenghuiwang The circular reference is QA test case for now. I do not have an example to use circular reference.

When there are complicated pre-defined schemas, it is hard for human to check circular reference and find which schema is wrong if circular reference comes up. The server crash with "stack overflow" message can not help user to know what happens and how to fix it quickly. I think it's the server's responsibility to help user check it and report the info to user.

when server starts, proactive schema check and a clear error message can help indeed instead of panic message.
@dreamryx just one comment. SchemaJSON check should be a common case. So I wonder if has existing related lib to do such check including close-loop check instead of implementating it by yourself here.

There is already a unit test to test loading the predefined schemas.

Schemas are predefined so that you can check errors before run time and the parsing code doesn't have to handle arbitrary schemas, e.g. ones with circular references. I think it is a fine assumption because it simplifies the parsing code and schema.

These newly added code will run during sever start but won't find anything.(otherwise something is wrong with the releasing process) It will just print a nicer error message when the unit test fails. To me that is not the right reason to complicate the production code.