code review 4550093: netchan: added drain method to importer.

netchan: added drain method to importer.

Fixes issue 1868.

[fritz, 2011-05-30 21:07:50]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-06-01 02:58:29]

this will need a test

http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go
File src/pkg/netchan/import.go (right):

http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:277: //Any unacknowledged messages?
s/A/ A/

http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:284: }
if imp.unackedCount() <= 0 {
  break
}

is all you need, and i'm not even sure i'd bother making unackedCount a
function, although it's not a problem to do so.

[fritz, 2011-06-01 23:54:23]

Hello golang-dev@googlegroups.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[fritz, 2011-06-01 23:54:57]

PTAL. I'll toss an update to the netchan test in another ticket.

http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go
File src/pkg/netchan/import.go (right):

http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:277: //Any unacknowledged messages?
On 2011/06/01 02:58:29, r wrote:
> s/A/ A/

Done.

http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:284: }
On 2011/06/01 02:58:29, r wrote:
> if imp.unackedCount() <= 0 {
>   break
> }
> 
> is all you need, and i'm not even sure i'd bother making unackedCount a
> function, although it's not a problem to do so.

Done. I'm leaving unackedCount() to accommodate a Sync() function.

[rsc, 2011-06-02 02:17:02]

You can put the test in the same CL.
Change the files and then run "hg change 4550093"
and add them to the list.

On Wed, Jun 1, 2011 at 19:54,  <david.jakob.fritz@gmail.com> wrote:
> PTAL. I'll toss an update to the netchan test in another ticket.
>
>
> http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go
> File src/pkg/netchan/import.go (right):
>
>
http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go#new...
> src/pkg/netchan/import.go:277: //Any unacknowledged messages?
> On 2011/06/01 02:58:29, r wrote:
>>
>> s/A/ A/
>
> Done.
>
>
http://codereview.appspot.com/4550093/diff/2001/src/pkg/netchan/import.go#new...
> src/pkg/netchan/import.go:284: }
> On 2011/06/01 02:58:29, r wrote:
>>
>> if imp.unackedCount() <= 0 {
>>   break
>> }
>
>> is all you need, and i'm not even sure i'd bother making unackedCount
>
> a
>>
>> function, although it's not a problem to do so.
>
> Done. I'm leaving unackedCount() to accommodate a Sync() function.
>
> http://codereview.appspot.com/4550093/
>

[fritz, 2011-06-02 23:22:44]

Hello golang-dev@googlegroups.com, r@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-06-05 11:27:31]

http://codereview.appspot.com/4550093/diff/8001/src/pkg/netchan/import.go
File src/pkg/netchan/import.go (right):

http://codereview.appspot.com/4550093/diff/8001/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:38: seqLock  sync.Mutex
these are not properly commented. also i'm surprised it takes four new fields
including two mutexes to do something so simple. at the very least, you could
use one (mutexed) integer to store the difference, because that's all you care
about: it's the count of unacked messages.

http://codereview.appspot.com/4550093/diff/8001/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:274: for {
for imp.unackedCount() > 0 {

[fritz, 2011-06-05 15:09:02]

Hello golang-dev@googlegroups.com, r@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[fritz, 2011-06-05 15:11:51]

http://codereview.appspot.com/4550093/diff/8001/src/pkg/netchan/import.go
File src/pkg/netchan/import.go (right):

http://codereview.appspot.com/4550093/diff/8001/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:38: seqLock  sync.Mutex
Fixed comments. 

Removed seqNum, changed ackNum to unackedNum, and made changes below.

I've left the seqLock mutex in, as the exporter guarantees that messages are
sent in order, and I assumed the same behavior would be wanted on the importer
side.

On 2011/06/05 11:27:31, r wrote:
> these are not properly commented. also i'm surprised it takes four new fields
> including two mutexes to do something so simple. at the very least, you could
> use one (mutexed) integer to store the difference, because that's all you care
> about: it's the count of unacked messages.

http://codereview.appspot.com/4550093/diff/8001/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:274: for {
On 2011/06/05 11:27:31, r wrote:
> for imp.unackedCount() > 0 {

Done.

[r, 2011-06-05 21:48:32]

http://codereview.appspot.com/4550093/diff/1002/src/pkg/netchan/import.go
File src/pkg/netchan/import.go (right):

http://codereview.appspot.com/4550093/diff/1002/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:35: unackedNum   int64		// number of unacknowledged
messages. < 0 is in error.
"Num" is one of those meaningless suffixes. "unacked" is fine.
the comment should mention it's only sent messages that we count.
you say <0 is an error but don't check that

unacked int64 // number of unacknowledged sends.

http://codereview.appspot.com/4550093/diff/1002/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:36: mu       sync.Mutex		// protects access to
unackedNum
move this up one line. see the usage in export.go

[fritz, 2011-06-06 01:27:55]

http://codereview.appspot.com/4550093/diff/1002/src/pkg/netchan/import.go
File src/pkg/netchan/import.go (right):

http://codereview.appspot.com/4550093/diff/1002/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:35: unackedNum   int64		// number of unacknowledged
messages. < 0 is in error.
On 2011/06/05 21:48:33, r wrote:
> "Num" is one of those meaningless suffixes. "unacked" is fine.
> the comment should mention it's only sent messages that we count.
> you say <0 is an error but don't check that
> 
> unacked int64 // number of unacknowledged sends.

Done.

http://codereview.appspot.com/4550093/diff/1002/src/pkg/netchan/import.go#new...
src/pkg/netchan/import.go:36: mu       sync.Mutex		// protects access to
unackedNum
On 2011/06/05 21:48:33, r wrote:
> move this up one line. see the usage in export.go

Done.

[fritz, 2011-06-06 01:28:38]

Hello golang-dev@googlegroups.com, r@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-06-06 06:45:24]

LGTM

[r, 2011-06-06 06:55:36]

*** Submitted as http://code.google.com/p/go/source/detail?r=80bef554c8df ***

netchan: added drain method to importer.

Fixes issue 1868.

R=golang-dev, r, rsc
CC=golang-dev
http://codereview.appspot.com/4550093

Committer: Rob Pike <r@golang.org>