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

Added overwrite parameter to catalog client.Put #285

Merged

Conversation

MorpheusXAUT
Copy link
Contributor

TL;DR

The catalog client's Put now has an additional parameter overwrite, indicating whether the artifact should be created (false) or updated (true).

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

The catalog client interface's Put was previously only used to create artifacts. In order to support the cache eviction RFC (and its first step of overwriting existing results for a single execution), an overwrite parameter was added, indicating the data store should replace an existing artifact instead of creating a new one.
As the current implementation is already called Put and the HTTP verb PUT is expected to replace existing data as well, I've opted for extending the current signature instead of creating a separate new method.

Tracking Issue

flyteorg/flyte#2867

Follow-up issue

NA

@welcome
Copy link

welcome bot commented Sep 27, 2022

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@codecov
Copy link

codecov bot commented Sep 27, 2022

Codecov Report

Merging #285 (ef1a529) into master (1637021) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head ef1a529 differs from pull request most recent head 0171adc. Consider uploading reports for the commit 0171adc to get more accurate results

@@            Coverage Diff             @@
##           master     #285      +/-   ##
==========================================
+ Coverage   63.32%   63.37%   +0.05%     
==========================================
  Files         145      145              
  Lines        9311     9324      +13     
==========================================
+ Hits         5896     5909      +13     
  Misses       2872     2872              
  Partials      543      543              
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
go/tasks/pluginmachinery/catalog/client.go 87.50% <ø> (ø)
go/tasks/plugins/k8s/pod/container.go 66.66% <ø> (ø)
go/tasks/pluginmachinery/flytek8s/pod_helper.go 79.13% <100.00%> (+0.18%) ⬆️
go/tasks/plugins/k8s/pod/plugin.go 84.88% <100.00%> (ø)
go/tasks/plugins/k8s/spark/spark.go 79.29% <100.00%> (+0.83%) ⬆️
go/tasks/plugins/webapi/snowflake/plugin.go 67.08% <100.00%> (ø)

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

hamersaw
hamersaw previously approved these changes Nov 2, 2022
@hamersaw hamersaw self-requested a review November 2, 2022 22:04
@hamersaw hamersaw self-requested a review November 2, 2022 22:38
@hamersaw hamersaw merged commit d95b86b into flyteorg:master Nov 4, 2022
@welcome
Copy link

welcome bot commented Nov 4, 2022

Congrats on merging your first pull request! 🎉

@MorpheusXAUT MorpheusXAUT deleted the cache-eviction-update-artifact branch November 4, 2022 14:24
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Added overwrite parameter to catalog client.Put

Signed-off-by: Nick Müller <[email protected]>

* Refactored catalog client artifact overwrite into separate Update method

Signed-off-by: Nick Müller <[email protected]>

* Updated flyteidl and flytestdlib to latest released versions

Signed-off-by: Nick Müller <[email protected]>

Signed-off-by: Nick Müller <[email protected]>
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.

2 participants