Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Purview Registry adapt Sql data model #404

Merged
merged 6 commits into from
Jul 4, 2022
Merged

New Purview Registry adapt Sql data model #404

merged 6 commits into from
Jul 4, 2022

Conversation

YihuiGuo
Copy link
Contributor

@YihuiGuo YihuiGuo commented Jun 27, 2022

Based on #387
Add purview registry.

Keep sql registry code unchanged to avoid conflict.
Conform to Registy interfaces, provide same method and data format.

NOTICE that :
This PR does not totally eliminate the risk of concurrent conflict.
In old version, we update all entities like
purviewclient.upload_entities([entity_batch])
In this PR, several changes has been applied to shrink the time window for a concurrent race condition to happen:

  1. For entity uploading, only one entity was filled into the batch. (This means, register 1 entity for 1 upload)
  2. Check for entity existence before uploading by qualifeidName+typeName. If the matching entity exists, we will further check if these entities are identical, if not (an "update" operation), registry api will return HTTP409 Conflict.

Limitations:
By applying changes above to purview registry, the time window for a race condition to happen shrinks, but not eliminated.
A complete change needs discussion with Purview team about recommended way to secure MVCC.

@YihuiGuo YihuiGuo changed the title Purview Registry New Purview Registry adapt Sql data model Jul 4, 2022
Copy link
Collaborator

@Yuqing-cat Yuqing-cat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make more e2e test

@blrchen blrchen added the safe to test Tag to execute build pipeline for a PR from forked repo label Jul 4, 2022
@Yuqing-cat Yuqing-cat merged commit 7620fc7 into feathr-ai:main Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants