Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: fetch child node executions for NodeExecution rows #72

Merged
merged 6 commits into from
Jun 3, 2020

Conversation

schottra
Copy link
Contributor

@schottra schottra commented Jun 2, 2020

One of the end goals of our dynamic task experience changes is that instead of the hierarchy in the table being NodeExecution -> TaskExecution/WorkflowExecution -> NodeExecution, we remove the intermediate entity and make it NodeExecution -> NodeExecution.

This creates some issues:

  • To determine if a NodeExecution can be expanded, we need to know if it has any children. There is no property on a NodeExecution that tells us that it has children. To determine that information, we need different behavior based on the node type.
    • For Task nodes (dynamic tasks), we will need to fetch the TaskExecutions created by the given NodeExecution. For each TaskExecution, we need to fetch its child NodeExecutions.
    • For Workflow nodes (specifically those that launch a separate WorkflowExecution), we need to fetch all NodeExecutions which are children of the given WorkflowExecution identifier.
  • For some types of Nodes, the children need to be grouped by retry attempt. At the moment, this only applies to Task nodes, but it's useful for the concept of "groups of children" to be generic across all types of Nodes, so that the rendering logic is reusable. Workflow nodes don't currently allow retries, but may in the future. So they will only ever yield one group of children.

To address those issues, I updated the rendering/fetching logic as follows:

  • Added a useChildNodeExecutions hook which will implement the necessary logic to fetch and group children for a given NodeExecution, varying the logic based on the type of node.
    • For Task nodes, we fetch all of the task executions, and then the node executions for each of those
    • For workflow nodes, we fetch the NodeExecution children for the workflow execution id.
    • The RequestConfig is piped through from the top level and used for all nested requests for NodeExecutions. This means that any filtering/sorting will apply to all levels of the list.
  • Updated NodeExecutionRow to only render an expander if the value returned from useChildNodeExecutions is an array with length greater than zero.
  • Refactored and exported some of the work functions for fetching NodeExecutions and TaskExecutions so that they can be composed by other hooks.

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2020

Codecov Report

Merging #72 into dynamic-task-updates will increase coverage by 0.15%.
The diff coverage is 76.11%.

Impacted file tree graph

@@                   Coverage Diff                    @@
##           dynamic-task-updates      #72      +/-   ##
========================================================
+ Coverage                 61.50%   61.66%   +0.15%     
========================================================
  Files                       363      364       +1     
  Lines                      5669     5713      +44     
  Branches                    847      851       +4     
========================================================
+ Hits                       3487     3523      +36     
- Misses                     2182     2190       +8     
Impacted Files Coverage Δ
...ecutions/ExecutionDetails/NodeExecutionDetails.tsx 83.87% <ø> (ø)
src/components/Executions/types.ts 100.00% <ø> (ø)
...components/Executions/useWorkflowExecutionState.ts 35.29% <0.00%> (-2.21%) ⬇️
src/components/hooks/useNodeExecutions.ts 47.61% <45.45%> (+4.76%) ⬆️
...Executions/ExecutionDetails/ExecutionNodeViews.tsx 58.33% <50.00%> (+1.81%) ⬆️
...ions/TaskExecutionsList/TaskExecutionsListItem.tsx 51.72% <50.00%> (ø)
...rc/components/Executions/useChildNodeExecutions.ts 82.14% <82.14%> (ø)
src/common/formatters.ts 95.58% <100.00%> (ø)
src/components/Cache/utils.ts 100.00% <100.00%> (ø)
...components/Executions/ExecutionDetails/contexts.ts 100.00% <100.00%> (ø)
... and 8 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 7a252f3...6dbd484. Read the comment docs.

Copy link

@podehnal podehnal left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM 👍

@schottra schottra merged commit d05ea7c into dynamic-task-updates Jun 3, 2020
@schottra schottra deleted the node-execution-fetching-refactor branch June 3, 2020 21:30
schottra added a commit that referenced this pull request Jun 29, 2020
* refactor: fetch child node executions to determine expandability

* fix: handling detection of children for sub-workflows as well

* fix: poor performance with object-hash on some identifiers

* docs: cleanup and docs for newly exposed functions

* test: ensure request config is used for all levels

* chore: remove unused import
schottra added a commit that referenced this pull request Jun 30, 2020
* refactor: fetch child node executions to determine expandability

* fix: handling detection of children for sub-workflows as well

* fix: poor performance with object-hash on some identifiers

* docs: cleanup and docs for newly exposed functions

* test: ensure request config is used for all levels

* chore: remove unused import
schottra added a commit that referenced this pull request Jun 30, 2020
* feat: adds more identifying information for node executions (#70)

* feat: show workflow node name beneath node execution ids

* feat: updating DetailsPanel info for NodeExecutions

* fix lint errors

* adding tests for new formatter function

* test: adding new test for NodeExecutionDetails

* test: adding test for new code in NodeExecutionsTable

* refactor: fetch child node executions for NodeExecution rows (#72)

* refactor: fetch child node executions to determine expandability

* fix: handling detection of children for sub-workflows as well

* fix: poor performance with object-hash on some identifiers

* docs: cleanup and docs for newly exposed functions

* test: ensure request config is used for all levels

* chore: remove unused import

* feat: Remove intermediate NodeExecutionsTable row content (#75)

* refactor: removing specialized rows and rendering only nodes

* refactor: moving contexts up to common folder

* refactor: use a data cache for nested node mapping

* refactor: update loading of workflow data

* fix: update usage of NodeExecutions in graph tab

* fix: update TaskExecutionDetails to use data cache

* fix: getting tests and stories working again

* chore: docs and cleanup

* test: use a more robust element query

* refactor: use filter instead of reduce

* docs: adding some missing function docs

* fix: cleanup for dynamic tasks refactoring (#76)

* test: creating dynamic task cases for NodeExecutionsTable stories

* fix: styling for child group labels

* fix: mock api context for NodeExecutionsTable stories

* test: mock nodeExecutionData endpoint

* chore: remove unused imports

* fix: extract nodes from subworkflows as well

* fix: adjust borders to make child groups more obvious

* refactor: checkpoint for getting the nesting styles correct

* refactor: adding logic for borders/spacing based on nesting/index

* fix: correct workflow execution table row styles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants