Skip to content

Conversation

@SwanSpouse
Copy link
Contributor

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 ploxiln mentioned this pull request Jan 13, 2020
@ploxiln
Copy link
Member

ploxiln commented Jan 13, 2020

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

@SwanSpouse
Copy link
Contributor Author

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
Copy link
Member

ploxiln commented Jan 16, 2020

done in #11

@ploxiln ploxiln closed this Jan 16, 2020
@SwanSpouse SwanSpouse deleted the depth branch January 22, 2020 14:42
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.

2 participants