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
Development

Successfully merging this pull request may close these issues.

3 participants