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

delete: add endpoint for deleting namespaces #2244

Merged
merged 2 commits into from
Nov 17, 2022

Conversation

mobuchowski
Copy link
Contributor

Add option to hide/soft delete namespace.

When namespace is hidden, all datasets and jobs contained by this namespace are also hidden. Namespace stops being shown on UI - frontend filters it by isHidden attribute, instead of filtering on backend. Reason for doing it this way is huge potential complexity of changing namespace contract - for example, a lot of methods check whether namespace exists before performing.

Deleted namespace is being undeleted when relevant OpenLineage event is received. This does not automatically undelete all the datasets and jobs in the namespace, only those that are received in this event.

Also, fixes bug where deleted child job (from an event with ParentRunFacet) wasn't getting deleted due to internal job name handling.

Closes: #2095

Signed-off-by: Maciej Obuchowski obuchowski.maciej@gmail.com

@mobuchowski mobuchowski changed the title Delete/namespace endpoint delete: add endpoint for deleting namespaces Nov 14, 2022
@boring-cyborg boring-cyborg bot added api API layer changes client/java web labels Nov 14, 2022
@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #2244 (a03a54d) into main (1d8334b) will not change coverage.
The diff coverage is n/a.

❗ Current head a03a54d differs from pull request most recent head 74abc92. Consider uploading reports for the commit 74abc92 to get more accurate results

@@            Coverage Diff            @@
##               main    #2244   +/-   ##
=========================================
  Coverage     76.72%   76.72%           
  Complexity     1147     1147           
=========================================
  Files           219      219           
  Lines          5318     5318           
  Branches        423      423           
=========================================
  Hits           4080     4080           
  Misses          763      763           
  Partials        475      475           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mobuchowski mobuchowski force-pushed the delete/namespace-endpoint branch from 3d324c7 to 603c147 Compare November 14, 2022 14:28
@boring-cyborg boring-cyborg bot added the docs label Nov 14, 2022
api/src/main/java/marquez/common/Utils.java Outdated Show resolved Hide resolved

jobs = client.listJobs(namespaceName);
assertThat(jobs).hasSize(1);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add some undelete scenario?

Copy link
Member

Choose a reason for hiding this comment

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

The test does check the number of datasets after the namespace is undeleted. But, looks like that's a new dataset added to the namespace and not the original datasets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the idea here is not to automatically undelete everything in the namespace.

api/src/main/java/marquez/db/DatasetDao.java Show resolved Hide resolved
api/src/main/java/marquez/db/JobDao.java Show resolved Hide resolved
@mobuchowski mobuchowski force-pushed the delete/namespace-endpoint branch 4 times, most recently from 72b4ec7 to cbcec83 Compare November 15, 2022 17:25
@wslulciuc wslulciuc mentioned this pull request Nov 15, 2022
7 tasks
api/src/main/java/marquez/common/Utils.java Outdated Show resolved Hide resolved
.findBy(name.getValue())
.orElseThrow(() -> new NamespaceNotFoundException(name));
datasetService.deleteByNamespaceName(namespace.getName().getValue());
jobService.deleteByNamespaceName(namespace.getName().getValue());
Copy link
Member

Choose a reason for hiding this comment

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

What if we soft delete the dataset and job, but an error happens right before we soft delete the namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only thing would be that visually empty namespace would appear on the UI, as this is filtered on the frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think we should have more resiliency about those type of failures? I think this operation is idempotent, as long as there are no followup events for the same namespace.

Copy link
Member

@wslulciuc wslulciuc Nov 17, 2022

Choose a reason for hiding this comment

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

I think it's fine as is. Perhaps I would have had NamespaceService.delete() soft delete the dataset and job within a transaction (in the DAO layer).

status:
patch: off

ignore:
- "api/src/main/java/marquez/db/migrations/V44_1__UpdateRunsWithJobUUID.java"
Copy link
Member

Choose a reason for hiding this comment

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

OOh, cool! Had no idea this was even an option 💯

@mobuchowski mobuchowski force-pushed the delete/namespace-endpoint branch 3 times, most recently from 8cc6eb7 to 342a1c6 Compare November 16, 2022 13:02
Copy link
Member

@wslulciuc wslulciuc left a comment

Choose a reason for hiding this comment

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

A much much requested feature! 💯 🥇

Signed-off-by: Maciej Obuchowski <obuchowski.maciej@gmail.com>
@mobuchowski mobuchowski force-pushed the delete/namespace-endpoint branch from 342a1c6 to b882daa Compare November 17, 2022 10:06
Signed-off-by: Maciej Obuchowski <obuchowski.maciej@gmail.com>
@mobuchowski mobuchowski force-pushed the delete/namespace-endpoint branch from b882daa to 74abc92 Compare November 17, 2022 10:14
@mobuchowski mobuchowski merged commit 0f857c5 into main Nov 17, 2022
@mobuchowski mobuchowski deleted the delete/namespace-endpoint branch November 17, 2022 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

api: add possibility to delete NAMESPACES
3 participants