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

Flyte node timeout is an usererror #114

Merged
merged 2 commits into from
Apr 16, 2020
Merged

Conversation

surindersinghp
Copy link
Contributor

@surindersinghp surindersinghp commented Apr 16, 2020

TL;DR

Node timeout error should be tagged as an user error

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

Tracking Issue

flyteorg/flyte#277

@codecov-io
Copy link

Codecov Report

Merging #114 into master will increase coverage by 1.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   50.01%   51.08%   +1.07%     
==========================================
  Files         128      129       +1     
  Lines        7966     8218     +252     
==========================================
+ Hits         3984     4198     +214     
- Misses       3594     3616      +22     
- Partials      388      404      +16     
Impacted Files Coverage Δ
pkg/controller/nodes/executor.go 59.95% <100.00%> (+0.58%) ⬆️
...kg/controller/nodes/subworkflow/launchplan/noop.go 83.33% <0.00%> (-1.29%) ⬇️
pkg/controller/controller.go 0.00% <0.00%> (ø)
pkg/compiler/admin.go 100.00% <0.00%> (ø)
pkg/controller/nodes/task/transformer.go 97.36% <0.00%> (+0.70%) ⬆️
pkg/controller/handler.go 91.42% <0.00%> (+1.89%) ⬆️
...g/controller/nodes/subworkflow/launchplan/admin.go 79.83% <0.00%> (+3.49%) ⬆️
pkg/controller/nodes/dynamic/handler.go 68.56% <0.00%> (+4.77%) ⬆️
pkg/controller/nodes/task/backoff/handler.go 79.46% <0.00%> (+5.55%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd3571d...8efff64. Read the comment docs.

@kumare3
Copy link
Contributor

kumare3 commented Apr 16, 2020

How did this improve coverage?

@EngHabu
Copy link
Contributor

EngHabu commented Apr 16, 2020

Unit test?

@EngHabu
Copy link
Contributor

EngHabu commented Apr 16, 2020

@kumare3 probably because this line was already being covered in a test, and since we increased the size of the block, it looked like more code (percentage) is covered

@surindersinghp
Copy link
Contributor Author

How did this improve coverage?

the line was already covered, and has been split from 1 into 2 :)

@surindersinghp surindersinghp merged commit 7028ee3 into master Apr 16, 2020
kumare3 pushed a commit to nuclyde-io/flytepropeller that referenced this pull request Feb 4, 2021
* introduce pr to update project repo

* wip

* wip

* wip + merge

* wip + compiles woot woot

* don't return types where unused; db write

* check for writeTx error

* interface

* use get

* add first integration test

* cmts

* fix

* new integration test

* ci

* apologies for all the commits; running CI off GitHub

* nit

* api update

* update mock interface

* wip

* mock update

* lol

* bump back some dependencies

* dep

* unit test and migration

* protobuf

* should be last dep change

* revert go.mode and make compile

* mock out more things, fix white space

* wip

* wip

* gofmt

* project

* Update pkg/repositories/gormimpl/project_repo.go

Co-authored-by: Katrina Rogan <krogan@lyft.com>

* Update pkg/repositories/gormimpl/project_repo.go

Co-authored-by: Katrina Rogan <krogan@lyft.com>

* cmnts

* truncate tables before tests

* update integration test

* integration test

* :=

Co-authored-by: Konstantin Gizdarski <kgizdarski@lyft.com>
Co-authored-by: Katrina Rogan <krogan@lyft.com>
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
* Flyte node timeout is an usererror

* Update executor.go
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.

4 participants