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(processor): stop using global variables #2881

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Conversation

atzoum
Copy link
Contributor

@atzoum atzoum commented Jan 17, 2023

Description

Preparation steps before starting the actual work for introducing workspace isolation in processor:

  • Removing global variables from the processor package
  • Renaming processor.HandleT to processor.Handle
  • Introducing misc.AsyncInit for streamlining how our components wait for their proper initialization before actually starting working (hint: backendconfig.WaitForConfig is not 100% safe)
  • Make sure that processor is properly initialized before starting (using misc.AsyncInit)
  • Fix issue with multiple successful HTTP requests performed for transformer features when starting up

Notion Ticket

Link

Security

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

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 49.66% // Head: 49.54% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (e37ab56) compared to base (44691c3).
Patch coverage: 86.66% of modified lines in pull request are covered.

❗ Current head e37ab56 differs from pull request most recent head 138fbc2. Consider uploading reports for the commit 138fbc2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2881      +/-   ##
==========================================
- Coverage   49.66%   49.54%   -0.13%     
==========================================
  Files         316      315       -1     
  Lines       50433    50108     -325     
==========================================
- Hits        25048    24826     -222     
+ Misses      23853    23765      -88     
+ Partials     1532     1517      -15     
Impacted Files Coverage Δ
processor/transformqueue.go 79.41% <ø> (+8.35%) ⬆️
runner/runner.go 70.61% <ø> (+0.22%) ⬆️
processor/trackingplan.go 53.38% <66.66%> (ø)
processor/manager.go 94.00% <84.61%> (+0.66%) ⬆️
processor/processor.go 86.15% <86.95%> (-0.22%) ⬇️
utils/misc/asyncinit.go 88.00% <88.00%> (ø)
...ger/transformation/transformationStatusUploader.go 24.24% <0.00%> (-14.61%) ⬇️
testhelper/log/log.go 7.69% <0.00%> (-3.85%) ⬇️
services/rsources/handler.go 73.82% <0.00%> (-1.38%) ⬇️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@atzoum atzoum force-pushed the refactor.processor branch 3 times, most recently from 4586717 to 15e9e5c Compare January 18, 2023 13:27
@atzoum atzoum marked this pull request as ready for review January 18, 2023 13:29
@atzoum atzoum force-pushed the refactor.processor branch 3 times, most recently from ab30819 to b6bee8b Compare January 18, 2023 14:20
processor/processor.go Outdated Show resolved Hide resolved
}

// AsyncInit is a helper object to wait for multiple asynchronous initialization events.
type AsyncInit struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like WaitContext is used in tests only. Why not use a WaitGroup instead of creating this new AsyncInit type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a wait group was my initial approach, but WaitGroup#Wait is a blocking call, whereas listening from a channel doesn't have to be. Consider this as a WaitGroup using a channel for communicating completion instead of blocking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, it's not too complex either so 👍

processor/processor.go Outdated Show resolved Hide resolved
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.

3 participants