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

Return ResourceWrapper without pointer #407

Closed
wants to merge 6 commits into from
Closed

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Sep 26, 2023

TL;DR

Worker in async cache panics because it fails to cast resource to *ResourceWrapper

cache (*Resource) -> cast to (*Resource) // it works

s3 (Resource) -> cast to (*Resource) // Failed to do type casting since we unmarshall the resource to Resource here.

We read the resource from s3 bucket after restarting the propeller (cache becomes empty).

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

^^^

Tracking Issue

NA

Follow-up issue

NA

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

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

Comparison is base (2598c96) 62.73% compared to head (94a9c38) 64.17%.
Report is 3 commits behind head on master.

❗ Current head 94a9c38 differs from pull request most recent head 9944aae. Consider uploading reports for the commit 9944aae to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
+ Coverage   62.73%   64.17%   +1.43%     
==========================================
  Files         156      156              
  Lines       13173    10709    -2464     
==========================================
- Hits         8264     6872    -1392     
+ Misses       4284     3204    -1080     
- Partials      625      633       +8     
Flag Coverage Δ
unittests ?

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

Files Coverage Δ
go/tasks/pluginmachinery/core/phase.go 21.50% <ø> (-1.27%) ⬇️
go/tasks/pluginmachinery/flytek8s/config/config.go 50.00% <ø> (ø)
...machinery/flytek8s/config/k8spluginconfig_flags.go 51.28% <100.00%> (+3.78%) ⬆️
go/tasks/plugins/array/k8s/subtask_exec_context.go 83.18% <100.00%> (+1.36%) ⬆️
go/tasks/plugins/k8s/ray/config_flags.go 38.70% <100.00%> (+2.34%) ⬆️
go/tasks/plugins/webapi/snowflake/plugin.go 66.25% <85.71%> (+4.95%) ⬆️
go/tasks/plugins/webapi/agent/plugin.go 67.95% <75.00%> (+1.90%) ⬆️
go/tasks/plugins/webapi/databricks/plugin.go 65.00% <75.00%> (+3.65%) ⬆️
go/tasks/plugins/k8s/ray/ray.go 84.17% <80.00%> (+5.83%) ⬆️
go/tasks/plugins/k8s/ray/config.go 36.36% <0.00%> (-13.64%) ⬇️

... and 128 files with indirect coverage changes

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

@eapolinario
Copy link
Contributor

eapolinario commented Oct 3, 2023

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to . Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

edit: Re-opening these PRs as the automation failed.

@eapolinario eapolinario closed this Oct 3, 2023
@eapolinario eapolinario reopened this Oct 3, 2023
@eapolinario
Copy link
Contributor

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to flyteorg/flyte#4132. Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

@eapolinario eapolinario closed this Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants