Skip to content

Conversation

pboothe
Copy link
Contributor

@pboothe pboothe commented Dec 21, 2018

Mmeoryless allows the running of functions as Poisson processes.


This change is Reviewable

Mmeoryless allows the running of functions as Poisson processes.
@coveralls
Copy link
Collaborator

coveralls commented Dec 21, 2018

Pull Request Test Coverage Report for Build 255

  • 33 of 33 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.5%) to 66.876%

Totals Coverage Status
Change from base Build 248: 2.5%
Covered Lines: 319
Relevant Lines: 477

💛 - Coveralls

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

:lgtm: one request to verify behavior of timer GC. Please don't submit without positively verifying.

Reviewed 3 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained


memoryless/memoryless.go, line 19 at r1 (raw file):

 Min <= Expected <= Max (or Min <= Expected and Max is 0)

Please arrange fields individually with some help text describing the condition above.


memoryless/memoryless.go, line 76 at r1 (raw file):

	// finished by the time the function is done" condition will not have the
	// potential to cause livelock for Run()
	done := false

I don't have a better answer, but want to ask: is this as simple as it can be?


memoryless/memoryless.go, line 84 at r1 (raw file):

		// Start the timer before the function call because the time between function
		// call *starts* should be exponentially distributed.
		t := time.NewTimer(c.waittime())

My understanding is that timers are managed by the Go runtime. And, the time.Tick docs warn of leaks when the "timer is not shutdown properly". Please verify that overwriting the variable is sufficient to release the resources of a new timer without calling .Stop.

Copy link
Contributor Author

@pboothe pboothe 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! 1 of 1 LGTMs obtained


memoryless/memoryless.go, line 19 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…
 Min <= Expected <= Max (or Min <= Expected and Max is 0)

Please arrange fields individually with some help text describing the condition above.

Done.


memoryless/memoryless.go, line 76 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I don't have a better answer, but want to ask: is this as simple as it can be?

No! Fixed!


memoryless/memoryless.go, line 84 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

My understanding is that timers are managed by the Go runtime. And, the time.Tick docs warn of leaks when the "timer is not shutdown properly". Please verify that overwriting the variable is sufficient to release the resources of a new timer without calling .Stop.

Tickers are Timers which never stop repeating. Leaking them is bad. Timers fire at most once and then exit. Leaking them is more okay. Added code to stop it anyway.

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

:lgtm: - one typo.

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained


memoryless/memoryless.go, line 23 at r2 (raw file):

	Min time.Duration
	// Max provides clamping of the randomly produced value. All timers will take
	// at least Max time.

typo: "at most".

Copy link
Contributor

@stephen-soltesz stephen-soltesz 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: :shipit: complete! 1 of 1 LGTMs obtained

@pboothe pboothe merged commit be52482 into master Dec 21, 2018
@pboothe pboothe deleted the memoryless branch December 21, 2018 20:22
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