Admin changes for Parent child node execution relationship #111
Conversation
anandswaminathan
requested review from
EngHabu,
katrogan,
kumare3,
matthewphsmith and
wild-endeavor
as code owners
anandswaminathan
force-pushed
the
parent-child
branch
from
72f0feb
to
1d71cae
Compare
|
PTAL @kumare3 @katrogan @wild-endeavor |
| var nodeExecutionMetadata admin.NodeExecutionMetaData | ||
| err = proto.Unmarshal(nodeExecutionModel.NodeExecutionMetadata, &nodeExecutionMetadata) | ||
| if err != nil { | ||
| return nil, errors.NewFlyteAdminErrorf(codes.Internal, "failed to unmarshal nodeExecutionMetadata") |
There was a problem hiding this comment.
@wild-endeavor Hey we do not log on the transformer layer. This error is logged in the manager layer already (caller of this method)
| // Parent that spawned this node execution - value is empty for executions at level 0 | ||
| ParentID *uint `sql:"default:null"` | ||
| // List of child node executions - for cases like Dynamic task, sub workflow, etc | ||
| ChildNodeExecutions []NodeExecution `gorm:"foreignkey:ParentID"` |
There was a problem hiding this comment.
because i'm dense, can you maybe explain to me what this means? usually in a has-many relationship, it means there are multiple rows of the thing that you have many of. in this case however, we're storing things as a list? or does it just mean that multiple children rows can have the same ParentID?
I think it'd be super helpful to see the db table before and after this change. Maybe I'll play around when I have some time.
There was a problem hiding this comment.
Multiple children rows will have the same ParentID
---------------------------+--------------------------+-----------+----------+---------------------------------------------
id | integer | | not null | nextval('node_executions_id_seq'::regclass)
created_at | timestamp with time zone | | |
updated_at | timestamp with time zone | | |
deleted_at | timestamp with time zone | | |
execution_project | text | | not null |
execution_domain | text | | not null |
execution_name | text | | not null |
node_id | text | | not null |
phase | text | | |
input_uri | text | | |
closure | bytea | | |
started_at | timestamp with time zone | | |
node_execution_created_at | timestamp with time zone | | |
node_execution_updated_at | timestamp with time zone | | |
duration | bigint | | |
parent_task_execution_id | integer | | |
node_execution_metadata | bytea | | |
parent_id | integer | | |
error_kind | text | | |
error_code | text | | |
cache_status | text | | |
Indexes:
"node_executions_pkey" PRIMARY KEY, btree (execution_project, execution_domain, execution_name, node_id)
"idx_node_executions_deleted_at" btree (deleted_at)
"idx_node_executions_error_kind" btree (error_kind)
"idx_node_executions_id" btree (id)
"idx_node_executions_node_id" btree (node_id)
"idx_node_executions_parent_task_execution_id" btree (parent_task_execution_id)
| @@ -77,9 +76,21 @@ func (m *NodeExecutionManager) createNodeExecutionWithEvent( | |||
| } | |||
| parentTaskExecutionID = taskExecutionModel.ID | |||
| } | |||
| var parentID *uint | |||
| if request.Event.ParentNodeMetadata != nil { | |||
| parentNodeExecutionModel, err := util.GetNodeExecutionModel(ctx, m.db, &core.NodeExecutionIdentifier{ | |||
There was a problem hiding this comment.
do we retrieve the entire model, just to link the ID? Can we not get just the "id"
There was a problem hiding this comment.
We can. The performance improvement is tiny, as node execution record size are small. In fact you would not see any difference.
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
==========================================
- Coverage 62.51% 62.42% -0.10%
==========================================
Files 104 104
Lines 7601 7649 +48
==========================================
+ Hits 4752 4775 +23
- Misses 2292 2312 +20
- Partials 557 562 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* Admin changes for Parent child node execution relationship * Add migrations
This PR has the following
Type
Are all requirements met?
Tracking Issue
flyteorg/flyte#218