Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Datacatalog delete artifact #353

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

MorpheusXAUT
Copy link
Contributor

TL;DR

This PR prepares datacatalog and flyteadmin endpoints for the second part of our cache eviction changes, evicting the cache of a (past) execution.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

A new CacheService was introduced to flyteadmin's proto definitions. The two currently defined methods allow for cache eviction for an Execution or TaskExecution. Initially, both endpoints were merged in a single endpoint (since handling in flyteadmin is very similar), however due to an issue in grpc-gateway < 2.0.1 with oneof properties and additional_bindings, I've had to split them into two separate endpoints. This could be be merged again once the dependency upgrade has completed, however too many breaking changes would be introduced at the moment.
The returned CacheEvictionErrors are intended to be displayed in the frontend to provide feedback about cache eviction to the user.

Additionally, datacatalog's API was extended by DeleteArtifact, allowing for an artifact to be deleted from the database/blob storage based on ID or tag.

Tracking Issue

flyteorg/flyte#2867

Follow-up issue

NA

@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #353 (e9fb728) into master (c1c892a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #353   +/-   ##
=======================================
  Coverage   73.71%   73.71%           
=======================================
  Files          18       18           
  Lines        1377     1377           
=======================================
  Hits         1015     1015           
  Misses        311      311           
  Partials       51       51           
Flag Coverage Δ
unittests 73.71% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
clients/go/admin/authtype_enumer.go 23.68% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@MorpheusXAUT
Copy link
Contributor Author

Rebased onto current master branch.

Nick Müller added 9 commits January 19, 2023 11:48
Added new CacheEvictionError message representing an error encountered during eviction of stored data
Added new UpdateTaskExecution endpoint for updating task executions, currently only supporting cache eviction

Signed-off-by: Nick Müller <[email protected]>
Ran go mod tidy

Signed-off-by: Nick Müller <[email protected]>
grpc-gateway parsing of URL params does not work for joined endpoint at the moment - fixed in major version upgrade
Added extra CacheEvictionErrorCode enum entries

Signed-off-by: Nick Müller <[email protected]>
…artifacts to datacatalog

Signed-off-by: Nick Müller <[email protected]>
@MorpheusXAUT MorpheusXAUT force-pushed the datacatalog-delete-artifact branch from 9cc1f12 to e9fb728 Compare January 19, 2023 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant