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

chore: pass context #3326

Merged
merged 40 commits into from
May 25, 2023
Merged

chore: pass context #3326

merged 40 commits into from
May 25, 2023

Conversation

achettyiitr
Copy link
Member

@achettyiitr achettyiitr commented May 12, 2023

Description

  • Pass context from UploadJob down to the Integrations package. One context is created during the UploadJob and passed down to the integrations.

Notion Ticket

https://www.notion.so/rudderstacks/make-sure-context-works-as-intended-1d128f7c18fa49acaf007ab82b20394b?pvs=4

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@achettyiitr achettyiitr reopened this May 12, 2023
@achettyiitr achettyiitr force-pushed the chore.warehouse-pass-context branch from 092e563 to 1149333 Compare May 12, 2023 17:45
@achettyiitr achettyiitr force-pushed the chore.warehouse-pass-context branch from 101dc9a to abec13e Compare May 12, 2023 18:28
@achettyiitr achettyiitr marked this pull request as draft May 12, 2023 18:32
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Patch coverage: 79.27% and project coverage change: +0.36 🎉

Comparison is base (4a94c15) 68.12% compared to head (1dd32b2) 68.48%.

❗ Current head 1dd32b2 differs from pull request most recent head 25a24d8. Consider uploading reports for the commit 25a24d8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3326      +/-   ##
==========================================
+ Coverage   68.12%   68.48%   +0.36%     
==========================================
  Files         329      329              
  Lines       53492    52710     -782     
==========================================
- Hits        36443    36101     -342     
+ Misses      14719    14270     -449     
- Partials     2330     2339       +9     
Impacted Files Coverage Δ
warehouse/admin.go 3.17% <0.00%> (ø)
warehouse/errors.go 89.47% <ø> (+3.75%) ⬆️
...ns/datalake/schema-repository/schema_repository.go 85.00% <ø> (ø)
warehouse/integrations/manager/manager.go 95.83% <ø> (ø)
warehouse/jobs/jobs.go 7.69% <0.00%> (ø)
warehouse/utils/utils.go 82.27% <ø> (ø)
warehouse/client/client.go 48.31% <33.33%> (ø)
warehouse/identities.go 19.40% <43.75%> (+2.55%) ⬆️
warehouse/integrations/datalake/datalake.go 60.00% <47.61%> (ø)
warehouse/integrations/mssql/mssql.go 64.07% <56.60%> (-0.06%) ⬇️
... and 29 more

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@achettyiitr achettyiitr force-pushed the chore.disintegrate-warehouse-integration-tests branch from 456a04d to 2519e14 Compare May 12, 2023 20:52
@achettyiitr achettyiitr force-pushed the chore.disintegrate-warehouse-integration-tests branch from 2519e14 to b6bcf57 Compare May 12, 2023 21:18
@achettyiitr achettyiitr force-pushed the chore.disintegrate-warehouse-integration-tests branch from b6bcf57 to 2bbc859 Compare May 13, 2023 08:55
Base automatically changed from chore.disintegrate-warehouse-integration-tests to master May 17, 2023 08:25
@achettyiitr achettyiitr force-pushed the chore.warehouse-pass-context branch from f389b14 to 942f2d3 Compare May 17, 2023 09:11
@achettyiitr achettyiitr marked this pull request as ready for review May 17, 2023 09:13
@achettyiitr achettyiitr force-pushed the chore.warehouse-pass-context branch from 19b232e to c8cb345 Compare May 17, 2023 09:27
warehouse/admin.go Show resolved Hide resolved
warehouse/api.go Outdated
return settings
}

// overrideWithEnv overrides the config keys in the fileManager settings
// with fallback values pulled from env. Only supported for S3 for now.
func overrideWithEnv(settings *filemanager.SettingsT) {
envConfig := filemanager.GetProviderConfigFromEnv(context.TODO(), settings.Provider)
func overrideWithEnv(ctx context.Context, settings *filemanager.SettingsT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The context seems to be passed to backendConfig.WaitForConfig() only. What if it gets canceled or it times out though? Do we want GetProviderConfigFromEnv to return an error?

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need a context propagated to this function?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we do, in case of cancelation it would stop waiting for the backend config which is a blocking call, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah true but that part of code kinda feels weird to me and probably something which no one would ever use.

Because from the UI he won't even be able to create a S3 destination without filling the required fields but we are waiting for config to get the workspaceID for a destination which isn't there. Not sure if I put it out here properly

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cisse21 I think this is about proper context propagation thus implicitly honouring such contexts. As far as I understand these calls would be part of the gRPC server. So what if the client disconnects during a gRPC request? I would expect the context to get canceled. In such scenarios, with proper ctx propagation, the backendConfig.WaitForConfig would correctly return immediately instead of blocking and creating a (temporary) leak:

		pkgLogger.Info("Waiting for backend config")
		select {
		case <-ctx.Done():
			return

So all considered, I think context propagation does make sense. What I'm not sure of is whether it makes sense to have the function return an error too so that it would properly be logged and handled upstream i.e.:

func overrideWithEnv(ctx context.Context, settings *filemanager.SettingsT) error {
    // ...

    return ctx.Err()
}

If yes then getFileManagerSettings can return an error as well which can be used by validateObjectStorage which already returns an error that is handled upstream.

@achettyiitr any thoughts as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

@fracasula I am also aligned with passing the context.

@achettyiitr achettyiitr merged commit 990a405 into master May 25, 2023
@achettyiitr achettyiitr deleted the chore.warehouse-pass-context branch May 25, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants