Skip to content

[ENV-40] debounce refresh requests with quietperiod - #10172

Merged
ndeloof merged 1 commit into
docker:v2from
ndeloof:debounce
Jan 18, 2023
Merged

ndeloof merged 1 commit into
docker:v2from
ndeloof:debounce

Conversation

@ndeloof

@ndeloof ndeloof commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

What I did
As we capture fs events, collect them to list services which need to be updated, with debounce and a quiet period

@ndeloof
ndeloof requested review from a team, StefanScherer, glours, laurazard, milas, nicksieger and ulyssessouza and removed request for a team January 13, 2023 12:35
@codecov

codecov Bot commented Jan 13, 2023 •

Copy link
Copy Markdown

Codecov Report

Base: 73.89% // Head: 72.79% // Decreases project coverage by -1.11% ⚠️

Coverage data is based on head (acbae37) compared to base (0b1c867).
Patch has no changes to coverable lines.

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

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10172      +/-   ##
==========================================
- Coverage   73.89%   72.79%   -1.11%     
==========================================
  Files           2        2              
  Lines         272      272              
==========================================
- Hits          201      198       -3     
- Misses         60       62       +2     
- Partials       11       12       +1     
Impacted Files Coverage Ξ”
pkg/e2e/framework.go 70.98% <0.00%> (-1.18%) ⬇️

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.

@milas milas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

πŸƒ

Comment thread pkg/compose/watch.go Outdated
case <-ctx.Done():
return
case service := <-input:
timer.Reset(interval)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we don't know if the timer has fired here, we need to stop it/drain first:

if !timer.Stop() {
	<-timer.C
}
timer.Reset(interval)

https://pkg.go.dev/time#Timer.Reset

Comment thread pkg/compose/watch.go Outdated
needRefresh := make(chan string)
eg.Go(func() error {
debounce(ctx, quietPeriod, needRefresh, func(services []string) {
fmt.Fprintf(s.stderr(), "Updating %s after changes were detected", strings.Join(services, ", "))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
fmt.Fprintf(s.stderr(), "Updating %s after changes were detected", strings.Join(services, ", "))
fmt.Fprintf(s.stderr(), "Updating %s after changes were detected\n", strings.Join(services, ", "))

(Missing \n)

Comment thread pkg/compose/watch_test.go
"gotest.tools/v3/assert"
)

func Test_debounce(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might take a look at Go libs out there to help with controlling timers deterministically in tests, as this is likely to become a problematic/failing test in CI otherwise

Tilt uses this library fairly extensively: https://github.com/jonboulle/clockwork

It provides a clock, which is either the real one (i.e. pass calls through to time.Time) or a fake one where you can control when/how it advances

@ndeloof
ndeloof force-pushed the debounce branch 4 times, most recently from 7b3905f to 4137940 Compare January 18, 2023 10:53
@ndeloof
ndeloof marked this pull request as draft January 18, 2023 15:01
@ndeloof
ndeloof force-pushed the debounce branch 2 times, most recently from cb7e7c1 to f67ddc1 Compare January 18, 2023 15:37
@ndeloof
ndeloof marked this pull request as ready for review January 18, 2023 15:38
@ndeloof
ndeloof force-pushed the debounce branch 2 times, most recently from fe4e453 to acbae37 Compare January 18, 2023 15:46
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

@glours glours left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread pkg/utils/set.go

package utils

type Set[T comparable] map[T]struct{}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

one baby step...

@ndeloof
ndeloof merged commit c15bf19 into docker:v2 Jan 18, 2023
@ndeloof
ndeloof deleted the debounce branch January 18, 2023 21:12
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