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

periodic: Add TriggerRun method. #2389

Merged

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Jan 24, 2019

This allows to run a task immediately after periodic creation,
or at any point in time when it is deemed necessary.


This change is Reviewable

@lukedirtwalker lukedirtwalker changed the title periodic: Add TriggerNow method. periodic: Add TriggerRun method. Jan 24, 2019
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)


go/lib/periodic/periodic.go, line 63 at r1 (raw file):

	cancelF  context.CancelFunc
	trigger  chan struct{}
	stoppedB bool

What does stoppedB stand for?

I assumed it is the boolean equivalent to chan stopped, but in Stop and Kill the two can get out of sync.


go/lib/periodic/periodic.go, line 120 at r1 (raw file):

	r.mtx.Lock()
	defer r.mtx.Unlock()
	if !r.stoppedB {

Wouldn't draining r.stop give you the same info here?

select {
case <-r.stop:
default:
    r.trigger <- struct{}{}
}

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @oncilla and @scrye)


go/lib/periodic/periodic.go, line 63 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

What does stoppedB stand for?

I assumed it is the boolean equivalent to chan stopped, but in Stop and Kill the two can get out of sync.

Done.


go/lib/periodic/periodic.go, line 120 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Wouldn't draining r.stop give you the same info here?

select {
case <-r.stop:
default:
    r.trigger <- struct{}{}
}

I think just having select either case should be fine.

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker, @oncilla, and @scrye)


go/lib/periodic/periodic.go, line 120 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think just having select either case should be fine.

I think you are right. It looks good to me.


go/lib/periodic/periodic.go, line 107 at r2 (raw file):

// the next execution will be in 3 minutes.
//
// If the task is currently running this function will block until the trigger was received.

Please add a comment clarifying that the function can return without a triggered run happening. Appending or the runner received a stop signal should be enough.

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @oncilla)

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @oncilla)

This allows to run a task immediately after periodic creation,
or at any point in time when it is deemed necessary.
Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla and @scrye)


go/lib/periodic/periodic.go, line 107 at r2 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Please add a comment clarifying that the function can return without a triggered run happening. Appending or the runner received a stop signal should be enough.

Done.

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker lukedirtwalker merged commit 76f682c into scionproto:master Jan 25, 2019
@lukedirtwalker lukedirtwalker deleted the pubPeriodicImprovements branch January 25, 2019 11:58
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