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

Bug/Feature: Dynamic parent node should be finalized earlier #94

Merged
merged 4 commits into from
Mar 24, 2020

Conversation

kumare3
Copy link
Contributor

@kumare3 kumare3 commented Mar 19, 2020

TL;DR

This is not really a bug, but it so happens that the Parent node is stuck in terminating until the entire dynamic node is finalized. Which may take hours depending on the subnodes. This fix enable s Dynamic node to be released and allow termination as soon as it is complete

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

Dynamic node now has an intermediate state that especially handles finalization of the parent node

Tracking Issue

flyteorg/flyte#209

Follow-up issue

NA

 - This will enable Dynamic node to be released and allow termination as
soon as it is complete
@kumare3 kumare3 requested a review from EngHabu March 19, 2020 02:12
@codecov-io
Copy link

codecov-io commented Mar 19, 2020

Codecov Report

Merging #94 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   49.71%   49.77%   +0.05%     
==========================================
  Files         127      127              
  Lines        7939     7948       +9     
==========================================
+ Hits         3947     3956       +9     
  Misses       3608     3608              
  Partials      384      384              
Impacted Files Coverage Δ
pkg/apis/flyteworkflow/v1alpha1/node_status.go 17.56% <ø> (ø)
pkg/controller/nodes/dynamic/handler.go 63.78% <100.00%> (+1.39%) ⬆️

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 4b17ac8...522ad76. Read the comment docs.

@surindersinghp
Copy link
Contributor

looks good.

EngHabu
EngHabu previously approved these changes Mar 19, 2020
@@ -129,6 +129,8 @@ func (d dynamicNodeTaskNodeHandler) handleDynamicSubNodes(ctx context.Context, n
return trns, newState, nil
}

// The State machine for a dynamic node is as follows
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to complete this paragraph ?

Copy link
Contributor Author

@kumare3 kumare3 Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh ya, fragmented brain! Done

@kumare3 kumare3 requested a review from EngHabu March 24, 2020 04:19
@kumare3 kumare3 merged commit 0c0d5cc into master Mar 24, 2020
kumare3 pushed a commit to nuclyde-io/flytepropeller that referenced this pull request Feb 4, 2021
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 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.

5 participants