Fixing cache overwrite metadata update

[hamersaw]

Tracking issue

Iterating on PR - #4550

Why are the changes needed?

Refer to linked PR

What changes were proposed in this pull request?

Refer to linked PR

How was this patch tested?

Refer to linked PR

Setup process

Refer to linked PR

Screenshots

Refer to linked PR

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

#4550

Docs link

Refer to linked PR

[codecov]

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (5d199a8) 59.03% compared to head (701bd4b) 59.02%.

Files	Patch %	Lines
datacatalog/pkg/manager/impl/artifact_manager.go	45.45%	5 Missing and 1 partial ⚠️
...acatalog/pkg/repositories/transformers/artifact.go	50.00%	2 Missing and 1 partial ⚠️
flytepropeller/pkg/controller/nodes/cache.go	50.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4617      +/-   ##
==========================================
- Coverage   59.03%   59.02%   -0.01%     
==========================================
  Files         622      622              
  Lines       52793    52816      +23     
==========================================
+ Hits        31164    31175      +11     
- Misses      19142    19150       +8     
- Partials     2487     2491       +4     

Flag	Coverage Δ
unittests	59.02% <50.00%> (-0.01%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pvditt]

@hamersaw

Did this work on your machine? The changes look fine but I'm not able to trace any calls to UpdateArtifact aside from the uncalled Update from propellor to persist the updates.

[hamersaw]

@hamersaw

Did this work on your machine? The changes look fine but I'm not able to trace any calls to UpdateArtifact aside from the uncalled Update from propellor to persist the updates.

Great catch! I am seeing the exact same issue, thanks for keeping me honest. Will trace this down.

[hamersaw]

@pvditt Seems I ported this over incorrectly on the fast cache
work. In the original PR that added the cache overwrite functionality we automatically call Update when cache overwrite is set. Internally, this creates the dataset if it doesn't exist. So I added this same logic here, performed local testing of all the scenarios I could think of, and I think this is correct now. Please take a look.

[ysysys3074]

@pvditt Seems I ported this over incorrectly on the fast cache work. In the original PR that added the cache overwrite functionality we automatically call Update when cache overwrite is set. Internally, this creates the dataset if it doesn't exist. So I added this same logic here, performed local testing of all the scenarios I could think of, and I think this is correct now. Please take a look.

hi @hamersaw sorry i just came back from vacation, sorry for delay in response regarding the PR and thanks for iterating on my earlier PR. And i just noticed in my company's internal repo, we still have pre_post_executor.go which calls Update method in propeller when overwrite cache is on, but it seems open source repo have removed that file and added cache.go instead. thanks for the fix here!