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

docs: delete assets api #78

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

luthfifahlevi
Copy link

@luthfifahlevi luthfifahlevi commented Aug 10, 2024

### Create Reference API for Delete Assets

Idea:
DeleteAssets as DELETE /v1/assets/: an async API that will return how many rows that will affect based on filter criteria, and delete those rows from PostgreSQL (assets and lineage) and Elasticsearch in asynchronous.

Request:

  • query_expr represent query expr for filter criteria: https://github.com/expr-lang/expr.
  • dry_run represent whether it only return how much affected rows (if set to true), or also perform deletion process in the background (default: false).

Response:
affected_rows represent how many rows that will affect based on filter criteria.

Current Approach:

  • Query Expr --> AST --> SQL query and ES query.
  • Use those query as WHERE clause in SQL and filter in ES query in deletion process, make this process in asynchronous.
  • Return the affected rows

@coveralls
Copy link

coveralls commented Aug 10, 2024

Pull Request Test Coverage Report for Build 10503587786

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.134%

Totals Coverage Status
Change from base Build 10093937236: 0.0%
Covered Lines: 6345
Relevant Lines: 7453

💛 - Coveralls

@luthfifahlevi luthfifahlevi self-assigned this Aug 10, 2024
| Name | Located in | Description | Required | Schema |
|------------|------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------| ------ |
| query_expr | body | query expression based on [expr-lang](https://expr-lang.org/) to filtering the assets that wants to be deleted. `refreshed_at`, `type`, and `service` identifiers must exist in the query. The `type` and `service` must using equals (`==`) or `IN` operator | Yes | string |
| dry_run | body | if set to true, then deletion should not proceed and only return the affected rows. Else, will perform deletion in the background (default) | No | string |
Copy link

Choose a reason for hiding this comment

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

can we change the part saying:

if set to true, then deletion should not proceed and only return the affected rows

into

if set to true, deletion will not be executed but the number of rows matching the query is returned

also, need to put what is the default value to make it explicit. currently a bit confusing. an example:

(default: false) if set to true, ...


| Name | Type | Description | Required |
|---------------|---------|-----------------------------------------------------|----------|
| affected_rows | integer | the numbers of assets that match the given query | No |
Copy link

Choose a reason for hiding this comment

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

do we need to add column required in this response?

@haveiss haveiss requested a review from irainia December 2, 2024 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants