Write workflow and node execution events asynchronously #174

katrogan · 2021-04-02T19:48:34Z

TL;DR

Write workflow and node execution events asynchronously

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Should hopefully improve Create{Workflow|Node}ExecutionEvent performance

Tracking Issue

flyteorg/flyte#891

Follow-up issue

NA

Signed-off-by: Katrina Rogan <[email protected]>

codecov · 2021-04-02T19:55:26Z

Codecov Report

Merging #174 (e1d0779) into master (af505fb) will increase coverage by 0.36%.
The diff coverage is 78.46%.

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   63.13%   63.49%   +0.36%     
==========================================
  Files         101      105       +4     
  Lines        7318     7331      +13     
==========================================
+ Hits         4620     4655      +35     
+ Misses       2115     2101      -14     
+ Partials      583      575       -8

Flag	Coverage Δ
unittests	`63.49% <78.46%> (+0.36%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/repositories/gormimpl/execution_repo.go	`72.72% <ø> (+5.31%)`	⬆️
pkg/runtime/application_config_provider.go	`3.84% <ø> (ø)`
pkg/repositories/gormimpl/node_execution_repo.go	`72.27% <57.14%> (+7.67%)`	⬆️
...nts/implementations/node_execution_event_writer.go	`72.72% <72.72%> (ø)`
...implementations/workflow_execution_event_writer.go	`72.72% <72.72%> (ø)`
pkg/manager/impl/execution_manager.go	`69.83% <83.33%> (+0.39%)`	⬆️
pkg/repositories/gormimpl/execution_event_repo.go	`83.33% <83.33%> (ø)`
...repositories/gormimpl/node_execution_event_repo.go	`83.33% <83.33%> (ø)`
pkg/manager/impl/node_execution_manager.go	`70.69% <100.00%> (+2.12%)`	⬆️
... and 3 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 af505fb...e1d0779. Read the comment docs.

kumare3 · 2021-04-02T20:53:32Z

pkg/async/events/implementations/node_execution_event_writer.go

+func NewNodeExecutionEventWriter(db repositories.RepositoryInterface) interfaces.NodeExecutionEventWriter {
+	return &nodeExecutionEventWriter{
+		db:     db,
+		events: make(chan admin.NodeExecutionEventRequest),


you should make a buffered channel?

You should and the length of the channel should be pretty large, else you will block sender. Right now this is a sync send

yes good catch, ty! not sure what a good buffer size would be, any suggestions @kumare3 ?

done & added a test for this case

pkg/async/events/implementations/workflow_execution_event_writer.go

pkg/async/events/implementations/node_execution_event_writer.go

pkg/async/events/implementations/workflow_execution_event_writer.go

pkg/async/events/implementations/node_execution_event_writer.go

kumare3 · 2021-04-02T20:58:37Z

I do not think this is achieving the async comm, Also, I think we should add enough documentation to explain the actual semantics of the API

Signed-off-by: Katrina Rogan <[email protected]>

pkg/async/events/implementations/node_execution_event_writer.go

kumare3 · 2021-04-03T04:11:49Z

Do we want to run a perf test before we commit this?

Signed-off-by: Katrina Rogan <[email protected]>

katrogan · 2021-04-06T19:45:17Z

@kumare3 no noticeable change, still coming at 15 mins although this time I see inserts into node execution events taking way less wait time comparatively

katrogan · 2021-04-08T00:46:28Z

@kumare before and after comparison for create execution event endpoint

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 · 2021-04-08T23:16:49Z

pkg/runtime/application_config_provider.go

@@ -56,7 +58,11 @@ func (p *ApplicationConfigurationProvider) GetDbConfig() interfaces.DbConfig {
 }

 func (p *ApplicationConfigurationProvider) GetTopLevelConfig() *interfaces.ApplicationConfig {
-	return flyteAdminConfig.GetConfig().(*interfaces.ApplicationConfig)
+	applicationConfig := flyteAdminConfig.GetConfig().(*interfaces.ApplicationConfig)
+	if applicationConfig.AsyncEventsBufferSize == 0 {


no need to do this, just set the default in the config like so
https://github.com/flyteorg/flytepropeller/blob/master/pkg/controller/config/config.go#L47

@kumare3 done, ty!

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 · 2021-04-09T05:23:41Z

pkg/runtime/application_config_provider.go

@@ -19,7 +19,12 @@ const domains = "domains"
 const externalEvents = "externalEvents"

 var databaseConfig = config.MustRegisterSection(database, &interfaces.DbConfigSection{})
-var flyteAdminConfig = config.MustRegisterSection(flyteAdmin, &interfaces.ApplicationConfig{})
+
+var defaultFlyteAdminConfig = &interfaces.ApplicationConfig{


this might be a time to create default configs for all the configs? Or if you want do it as a follow up PR?

considering how many configs we have... how about a follow-up pr :)

flyteorg/flyte#904

kumare3 · 2021-04-09T05:24:24Z

looks good to me, one comment

katrogan · 2021-04-09T16:25:04Z

@kumare3 can you approve?

Signed-off-by: Haytham Abuelfutuh <[email protected]>

…D and OAuth2 configs, OAuth2 Metadata over gRPC #minor (#168) * wip: OAuth2 Support Signed-off-by: Haytham Abuelfutuh <[email protected]> * wip Signed-off-by: Haytham Abuelfutuh <[email protected]> * wip Signed-off-by: Haytham Abuelfutuh <[email protected]> * tighten security of generated tokens Signed-off-by: Haytham Abuelfutuh <[email protected]> * Support storing form post values in auth code JWT Signed-off-by: Haytham Abuelfutuh <[email protected]> * save secrets to k8s secrets Signed-off-by: Haytham Abuelfutuh <[email protected]> * Expose metadata endpoints over gRPC Signed-off-by: Haytham Abuelfutuh <[email protected]> * trim OpenID Connect config further Signed-off-by: Haytham Abuelfutuh <[email protected]> * Selectively authenticate gRPC endpoints Signed-off-by: Haytham Abuelfutuh <[email protected]> * Support external oauth2 server and Okta Config Signed-off-by: Haytham Abuelfutuh <[email protected]> * update config Signed-off-by: Haytham Abuelfutuh <[email protected]> * Fix nil secrets data map Signed-off-by: Haytham Abuelfutuh <[email protected]> * Fixed the pointer overwrite issue in oauthServer metadata (#183) Signed-off-by: Prafulla Mahindrakar <[email protected]> Co-authored-by: Prafulla Mahindrakar <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]> * Unit tests Signed-off-by: Haytham Abuelfutuh <[email protected]> * Unit tests Signed-off-by: Haytham Abuelfutuh <[email protected]> * Simplify config further and move auth package up Signed-off-by: Haytham Abuelfutuh <[email protected]> * Fix clusterresource Project and domain(#167) * Fix clusterresource Project Signed-off-by: Anand Swaminathan <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]> * Bump flyteidl version to pick up auth role field number fix (#169) Signed-off-by: Katrina Rogan <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]> * Add option to use project name as namespace for the task pods (#166) * Add option to use project name as namespace for the task pods Signed-off-by: Jeev B <[email protected]> * rename Signed-off-by: Jeev B <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]> * GetExecution performance improvements (#171) Signed-off-by: Katrina Rogan <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]> * Add exists check for workflow & node executions (#172) Signed-off-by: Haytham Abuelfutuh <[email protected]> * Remove legacy fetch for workflow execution inputs (#173) Signed-off-by: Haytham Abuelfutuh <[email protected]> * Added release workflow (#170) Signed-off-by: yuvraj <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]> * Update Flyteidl version (#175) Signed-off-by: Haytham Abuelfutuh <[email protected]> * Added version in flyteadmin (#154) * wip: added version pkg Signed-off-by: yuvraj <[email protected]> * wip: resolve conflict Signed-off-by: yuvraj <[email protected]> * wip: added version in rpc Signed-off-by: yuvraj <[email protected]> * wip: small fixes Signed-off-by: yuvraj <[email protected]> * wip: Added panic cache in get version service Signed-off-by: yuvraj <[email protected]> * Added flytestdlib for version package Signed-off-by: yuvraj <[email protected]> * Added version service test Signed-off-by: yuvraj <[email protected]> * wip: added ldflags in goreleaser Signed-off-by: yuvraj <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]> * Propagate nesting and principal for child executions (#177) Signed-off-by: Haytham Abuelfutuh <[email protected]> * Write workflow and node execution events asynchronously (#174) Signed-off-by: Haytham Abuelfutuh <[email protected]> * Add sensible flyteadmin config defaults (#179) Signed-off-by: Haytham Abuelfutuh <[email protected]> * Lint Signed-off-by: Haytham Abuelfutuh <[email protected]> * further cleanup Signed-off-by: Haytham Abuelfutuh <[email protected]> * Only register authserver when auth is enabled Signed-off-by: Haytham Abuelfutuh <[email protected]> * Update to latest flyteidl and separate auth interfaces Signed-off-by: Haytham Abuelfutuh <[email protected]> * dead code Signed-off-by: Haytham Abuelfutuh <[email protected]> * PR Comments Signed-off-by: Haytham Abuelfutuh <[email protected]> * merge master Signed-off-by: Haytham Abuelfutuh <[email protected]> * Move to authorizedUris Signed-off-by: Haytham Abuelfutuh <[email protected]> * Update to released flyteidl Signed-off-by: Haytham Abuelfutuh <[email protected]> * Fix response expiry and add unit tests Signed-off-by: Haytham Abuelfutuh <[email protected]> * Update go mod Signed-off-by: Haytham Abuelfutuh <[email protected]> * fix unit tests that broke because of identity changes Signed-off-by: Haytham Abuelfutuh <[email protected]> Co-authored-by: pmahindrakar-oss <[email protected]> Co-authored-by: Prafulla Mahindrakar <[email protected]> Co-authored-by: Anand Swaminathan <[email protected]> Co-authored-by: Katrina Rogan <[email protected]> Co-authored-by: Jeev B <[email protected]> Co-authored-by: Yuvraj <[email protected]> Co-authored-by: Flyte Bot <[email protected]>

async

4e5e3b6

Signed-off-by: Katrina Rogan <[email protected]>

katrogan requested review from anandswaminathan, EngHabu, kumare3 and wild-endeavor as code owners April 2, 2021 19:48

kumare3 reviewed Apr 2, 2021

View reviewed changes

pkg/async/events/implementations/workflow_execution_event_writer.go Outdated Show resolved Hide resolved

kumare3 reviewed Apr 2, 2021

View reviewed changes

pkg/async/events/implementations/node_execution_event_writer.go Show resolved Hide resolved

kumare3 reviewed Apr 2, 2021

View reviewed changes

pkg/async/events/implementations/workflow_execution_event_writer.go Show resolved Hide resolved

kumare3 reviewed Apr 2, 2021

View reviewed changes

pkg/async/events/implementations/node_execution_event_writer.go Show resolved Hide resolved

katrogan added 2 commits April 2, 2021 14:51

buffer

32daee1

Signed-off-by: Katrina Rogan <[email protected]>

noop

e9e51ac

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 reviewed Apr 3, 2021

View reviewed changes

pkg/async/events/implementations/node_execution_event_writer.go Show resolved Hide resolved

buffer size

963d084

Signed-off-by: Katrina Rogan <[email protected]>

katrogan added 2 commits April 8, 2021 16:13

configurable

e1b0db4

Signed-off-by: Katrina Rogan <[email protected]>

comment

08c87ae

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 reviewed Apr 8, 2021

View reviewed changes

katrogan added 2 commits April 8, 2021 16:17

merge

2a8744b

Signed-off-by: Katrina Rogan <[email protected]>

default config

e1d0779

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 reviewed Apr 9, 2021

View reviewed changes

kumare3 closed this Apr 9, 2021

kumare3 reopened this Apr 9, 2021

kumare3 self-requested a review April 9, 2021 17:40

kumare3 approved these changes Apr 9, 2021

View reviewed changes

katrogan merged commit 40d6d5c into master Apr 9, 2021

EngHabu pushed a commit that referenced this pull request Apr 26, 2021

Write workflow and node execution events asynchronously (#174)

c4efdc2

Signed-off-by: Haytham Abuelfutuh <[email protected]>

EngHabu pushed a commit that referenced this pull request Apr 26, 2021

Write workflow and node execution events asynchronously (#174)

2b02791

Signed-off-by: Haytham Abuelfutuh <[email protected]>

eapolinario pushed a commit that referenced this pull request Sep 6, 2023

Write workflow and node execution events asynchronously (#174)

f4f7acf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write workflow and node execution events asynchronously #174

Write workflow and node execution events asynchronously #174

katrogan commented Apr 2, 2021

codecov bot commented Apr 2, 2021 •

edited

Loading

kumare3 Apr 2, 2021

kumare3 Apr 2, 2021

katrogan Apr 2, 2021

katrogan Apr 2, 2021

kumare3 commented Apr 2, 2021

kumare3 commented Apr 3, 2021

katrogan commented Apr 6, 2021

katrogan commented Apr 8, 2021

kumare3 Apr 8, 2021

katrogan Apr 8, 2021

kumare3 Apr 9, 2021

katrogan Apr 9, 2021

katrogan Apr 9, 2021

kumare3 commented Apr 9, 2021

katrogan commented Apr 9, 2021

Write workflow and node execution events asynchronously #174

Write workflow and node execution events asynchronously #174

Conversation

katrogan commented Apr 2, 2021

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented Apr 2, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumare3 commented Apr 2, 2021

kumare3 commented Apr 3, 2021

katrogan commented Apr 6, 2021

katrogan commented Apr 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumare3 commented Apr 9, 2021

katrogan commented Apr 9, 2021

codecov bot commented Apr 2, 2021 •

edited

Loading