nsqio / go-diskqueue

A Go package providing a filesystem-backed FIFO queue
MIT License
467 stars 101 forks source link

change dq.Depth() to request & response model #10

Closed SwanSpouse closed 4 years ago

SwanSpouse commented 4 years ago
for i := 0; i < 10; i++ {
    err := dq.Put(msg)
    Nil(t, err)
    Equal(t, int64(i+1), dq.Depth())
}

for i := 10; i > 0; i-- {
    <-dq.ReadChan()
    fmt.Println(dq.Depth())
}

When i run such codes above, i get result like this.

10
8
8
6
6
4
4
2
2
0
line638: case r <- dataRead:
line639:    count++
line640:    // moveForward sets needSync flag if a file is removed
line641:    d.moveForward()

when executes at line638, <-dq.ReadChan will get result and continue, then we got wrong depth, because line641 d.moveForward() is not reached.

I send a signal to notice i want to get depth, it will run with <-dataRead in serial and get expected result.

ploxiln commented 4 years ago

That's an interesting observation, thanks ...

I think that the primary user of this library, nsqd, is OK with the fact that this Depth() call is "eventually consistent", because in nsqd it is (mostly? I think?) called from a different goroutine than the one reading or writing the diskqueue.

Changing Depth() to use channels like this makes it significantly more expensive. It may or may not matter, depending on how frequently the program using this calls Depth(). I came up with another solution, which I think is less expensive than your proposal (but still more expensive than just an atomic read): https://github.com/nsqio/go-diskqueue/pull/11/commits/b8887fdf997993a6be72c02ba7188af0cbcda1ee

I'll leave this for a little while, for the other maintainers to have some time to take a look and think about it. cc @mreiferson

SwanSpouse commented 4 years ago

That's an interesting observation, thanks ...

I think that the primary user of this library, nsqd, is OK with the fact that this Depth() call is "eventually consistent", because in nsqd it is (mostly? I think?) called from a different goroutine than the one reading or writing the diskqueue.

Changing Depth() to use channels like this makes it significantly more expensive. It may or may not matter, depending on how frequently the program using this calls Depth(). I came up with another solution, which I think is less expensive than your proposal (but still more expensive than just an atomic read): b8887fd

I'll leave this for a little while, for the other maintainers to have some time to take a look and think about it. cc @mreiferson

I agree, but as an independent library, it may be used in somewhere else, i think it's necessary to make Depth() return correct result even though it's more expensive.

your solution is much better and succinct. i benefit a lot! 😃

ploxiln commented 4 years ago

done in #11