d2iq-archive / mesos-dns

DNS-based service discovery for Mesos.
https://mesosphere.github.com/mesos-dns
Apache License 2.0
483 stars 137 forks source link

fix: add WaitGroup to accurately measure elapsed time of benchmark #534

Closed jaredfolkins closed 5 years ago

jaredfolkins commented 5 years ago

Howdy!

I took a look through this repository and found a small error. I figured in the spirit of saying "hello 👋" I'd ship a fix.

Thanks, Jared

What version of the project are you using?

Master repo - dcos 1.12

What operating system and processor architecture are you using?

Debian/Linux

What did you do?

Tried to test a benchmark

What did you expect to see?

The tool is asserting that it is benchmarking queries.

What did you see instead?

The tool simply iterates a for loop issuing 10k goroutines, which promptly close. The test isn't performing as desired.

jaredfolkins commented 5 years ago

Sample application.

package main

import (

        "log"

        "sync"

        "time"

)

func main() {

        wg := &sync.WaitGroup{}

        start := time.Now()

        cnt := 10000

        for i := 0; i < cnt; i++ {

                sleepy(5, wg)

        }

        wg.Wait()

        elapsed := time.Since(start)

        log.Printf("benching took %s", elapsed)

        log.Printf("doing %d/%v rps", cnt, elapsed)

}

func sleepy(t int, wg *sync.WaitGroup) {

        wg.Add(1)

        go func() {

                time.Sleep(time.Duration(t) * time.Second)

                wg.Done()

        }()

}
jdef commented 5 years ago

test this please

jdef commented 5 years ago

retest this please

mesosphere-ci commented 5 years ago

this is a test from jenkins

jdef commented 5 years ago

retest this please

jaredfolkins commented 5 years ago

I'm assuming this is all CI related? Were you needing me to do something?

jdef commented 5 years ago

Was trying to bump CI. Looks like a bigger CI change is needed. No input from you needed for now. Thanks.

On Wed, Feb 13, 2019, 5:49 PM Jared Folkins <notifications@github.com wrote:

I'm assuming this is all CI related? Were you needing me to do something?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mesosphere/mesos-dns/pull/534#issuecomment-463407617, or mute the thread https://github.com/notifications/unsubscribe-auth/ACPVLBSloZrnEHRA32WF7NYAMHgr_4scks5vNJZqgaJpZM4a1M9e .

jdef commented 5 years ago

@jaredfolkins would you mind rebasing, in order to exercise our updated CI tooling?

jaredfolkins commented 5 years ago

I'm on a deadline today but I'll loop back around to this by Tuesday of next week. Jf

On Fri, Feb 15, 2019, 6:31 AM James DeFelice <notifications@github.com wrote:

@jaredfolkins https://github.com/jaredfolkins would you mind rebasing, in order to exercise our updated CI tooling?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mesosphere/mesos-dns/pull/534#issuecomment-464069741, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKzDBQRmvzowHW6UEOFxolU4-3YsEkXks5vNsSngaJpZM4a1M9e .

jaredfolkins commented 5 years ago

Whoops. Was doing too many things at once. Let me clean this up.

jaredfolkins commented 5 years ago

Alright, that should have cleaned up the history. Let me know.

jaredfolkins commented 5 years ago

Hey, just curious your team's procedure for tickling the CI. Is it simply git commit --amend as that would change the SHA-1 hash? Maybe there is a better way and I always am learning.

jdef commented 5 years ago

That will do it. Public PRs also require approval from an owner before CI will build them.

On Sat, Feb 16, 2019, 4:31 PM Jared Folkins <notifications@github.com wrote:

Hey, just curious your team's procedure for tickling the CI. Is it simply git commit --amend as that would change the SHA-1 hash? Maybe there is a better way and I always am learning.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mesosphere/mesos-dns/pull/534#issuecomment-464387735, or mute the thread https://github.com/notifications/unsubscribe-auth/ACPVLBHeKr-lKqn_1jR1of5LvEUYH9zrks5vOHiwgaJpZM4a1M9e .