Issue 6782140: code review 6782140: log/syslog: retry once if write fails (Closed)
|
Created:
12 years, 3 months ago by jeff.allen Modified:
12 years, 1 month ago Reviewers:
CC:
mikio, rsc, bradfitz, golang-dev Visibility:
Public. |
Descriptionlog/syslog: retry once if write fails
Implements deferred connections + single-attempt automatic
retry. Based on CL 5078042 from kuroneko.
Fixes issue 2264.
Patch Set 1 #Patch Set 2 : diff -r 697f36fec52c https://code.google.com/p/go #Patch Set 3 : diff -r 697f36fec52c https://code.google.com/p/go #Patch Set 4 : diff -r 697f36fec52c https://code.google.com/p/go #
Total comments: 4
Patch Set 5 : diff -r 025b9d070a85 https://code.google.com/p/go #Patch Set 6 : diff -r 6e0e4077f488 https://code.google.com/p/go #Patch Set 7 : diff -r 52c9c412f1f2 https://code.google.com/p/go #
Total comments: 1
Patch Set 8 : diff -r a5efcd1675eb https://code.google.com/p/go #
Total comments: 11
Patch Set 9 : diff -r a5efcd1675eb https://code.google.com/p/go #Patch Set 10 : diff -r 477b2e70b12d https://go.googlecode.com/hg/ #
Total comments: 3
Patch Set 11 : diff -r 3fe40a41018d https://code.google.com/p/go #
Total comments: 13
Patch Set 12 : diff -r ee5afd4b14b7 https://code.google.com/p/go #
Total comments: 8
Patch Set 13 : diff -r 3684de5292bf https://code.google.com/p/go #Patch Set 14 : diff -r 1399878c6731 https://code.google.com/p/go #Patch Set 15 : diff -r f6172d444cc0 http://code.google.com/p/go #
Total comments: 5
Patch Set 16 : diff -r 9ff35ff05de6 http://code.google.com/p/go #
Total comments: 8
Patch Set 17 : diff -r 8d4bd93dcd41 http://code.google.com/p/go #
Total comments: 9
Patch Set 18 : diff -r a9211b512258 http://code.google.com/p/go #
MessagesTotal messages: 38
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
thank you for tackling this. https://codereview.appspot.com/6782140/diff/6001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/6001/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:1: // Copyright 2012 The Go Authors. All rights reserved. no need to update copyright line. https://codereview.appspot.com/6782140/diff/6001/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:18: "syscall" well, please have a look at src/pkg/go/build/deps_test.go. also please run all.bash before review. // pkgDeps defines the expected dependencies between packages in // the Go source tree. It is a statement of policy. // Changes should not be made to this map without prior discussion. // // The map contains two kinds of entries: // 1) Lower-case keys are standard import paths and list the // allowed imports in that package. // 2) Upper-case keys define aliases for package sets, which can then // be used as dependencies by other rules. // // DO NOT CHANGE THIS DATA TO FIX BUILDS. https://codereview.appspot.com/6782140/diff/6001/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:122: if w.conn != nil { no lock? https://codereview.appspot.com/6782140/diff/6001/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:147: return oe.Err == syscall.ECONNREFUSED platform independent?
Sign in to reply to this message.
OK, I think I understand the problem. log/syslog is allowed to use L4, OS and net. So it seems like I should be checking something in os or net instead of syscall. So it seems like I will need to add the concept of conn refused in net, following the pattern of Timeout(), perhaps. Sound like the right general direction to go?
Sign in to reply to this message.
Please take a look. This is what it would look like fi we did this by making a new OS-independent API to detect the connection refused condition. I need feedback from Go hackers if this is the right direction to go with this. Thanks.
Sign in to reply to this message.
Please don't edit the copyright notices: we don't bother updating the year. I agree with Mikio: I don't understand why we only want to do this is the connection was refused. What if the connection is unreachable because the machine is temporarily off the network? If this code is going to retry I think it should just retry, regardless of why it could not connect. Russ
Sign in to reply to this message.
Hi,
I don't see any reason we should tweak net package here.
func (w *Writer) writeString() (int, error) {
n, err := w.c.WriteString()
if err != nil {
if err := openlog(...); err != nil { // try to reconnect or reopen log
return 0, err
}
n, err := w.c.WriteString()
}
return n, err
}
isn't enough?
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
If you want to make a conn of Writer mutable then you
need to prevent data race. For example, go test -race
berks with attached test case.
==================
WARNING: DATA RACE
Write by goroutine 6:
log/syslog.(*Writer).reconnect()
/Users/go/src/pkg/log/syslog/syslog.go:135 +0x1fe
log/syslog.(*Writer).writeString()
/Users/go/src/pkg/log/syslog/syslog.go:214 +0x51
log/syslog.(*Writer).Write()
/Users/go/src/pkg/log/syslog/syslog.go:146 +0x91
io.WriteString()
/Users/go/src/pkg/io/io.go:256 +0xf2
log/syslog.func·001()
/Users/go/src/pkg/log/syslog/syslog_test.go:254 +0x7c
func TestConcurrentWrite(t *testing.T) {
addr := startUdpServer(make(chan string))
w, err := Dial("udp", addr, LOG_USER|LOG_ERR, "how's it going?")
if err != nil {
t.Fatalf("syslog.Dial() failed: %s", err)
}
var wg sync.WaitGroup
for i := 0; i < 10; i++ {
go func(w *Writer) {
wg.Add(1)
defer func() { wg.Done() }()
_, err = io.WriteString(w, "so so")
if err != nil {
t.Errorf("WriteString() failed: %s", err)
return
}
}(w)
}
wg.Wait()
}
Sign in to reply to this message.
Thanks, I totally forgot about that comment from before. I included your test and the correct locking code in syslog.go. I found something funny... even your test had a data race, because the err inside the go routine is shadowed between all the goroutines! Added a : to fix that right up. PTAL.
Sign in to reply to this message.
Keep deracing!
Sign in to reply to this message.
On Fri, Dec 7, 2012 at 7:37 PM, <dvyukov@google.com> wrote: > Keep deracing! Great work, much appreciate it.
Sign in to reply to this message.
I had a quick look ti syslog.go, not enough to tests. Also you don't need to update api.txt in this CL. https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:119: return &Writer{priority: priority, tag: tag, hostname: hostname, conn: nil, network: network, raddr: raddr}, nil why did you remove net.Dial from here? https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:122: // must be called with w.mu held looks like a different comment style. https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:219: w.mu.Lock() why not give a chance of first write before entering lock contention? https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:222: if w.conn == nil { test w.conn here then test again in reconnect looks redundant. https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog_t... File src/pkg/log/syslog/syslog_test.go (right): https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:35: func startUdpServer(done chan<- string) string { s/Udp/UDP/ https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:88: t.Fatalf("Dial() failed: %s", err) %v?
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com, rsc@golang.org, dvyukov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): http://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:119: return &Writer{priority: priority, tag: tag, hostname: hostname, conn: nil, network: network, raddr: raddr}, nil I am working from a previous patch, which implemented delayed connect at the same time as reconnect. It seemed like a good idea to me, so I kept it. http://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:122: // must be called with w.mu held On 2012/12/07 11:53:25, mikio wrote: > looks like a different comment style. Done. http://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:219: w.mu.Lock() My understanding is that reading w.conn is not safe unless I hold the mutex. Is only writing racy? http://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.go... src/pkg/log/syslog/syslog.go:222: if w.conn == nil { This test conn == nil. The other is conn != nil, to prevent a nil deref on w.conn.Close().
Sign in to reply to this message.
https://codereview.appspot.com/6782140/diff/34001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/34001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:88: type serverConn interface { serverConn? I'm confused a bit. https://codereview.appspot.com/6782140/diff/34001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:122: // *Writer.connect makes a connection to the syslog server. no *Writer prefix. https://codereview.appspot.com/6782140/diff/34001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:220: w.mu.Lock() sync.RWMutex is your friend.
Sign in to reply to this message.
https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/27001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:119: return &Writer{priority: priority, tag: tag, hostname: hostname, conn: nil, network: network, raddr: raddr}, nil I don't think it's a good idea. What if we have multiple syslog servers and want to implement candidate selection with own policies? Your dial takes all and we cannot take it back anymore.
Sign in to reply to this message.
https://codereview.appspot.com/6782140/diff/20001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/20001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:117: return &Writer{priority: priority, tag: tag, hostname: hostname, conn: nil, network: network, raddr: raddr}, nil The doc comment says that Dial establishes a connection, but now the code does not. Please make the code establish a connection once, so that Dial can return an error if the initial connection fails.
Sign in to reply to this message.
Hello mikioh.mikioh@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:82: mu sync.Mutex either put a space before this line and/or add a comment on the same line after sync.Mutex saying what it guards: mu sync.Mutex // guards conn, network, raddr or mu sync.Mutex // guards conn (in which case network and raddr should go above the mutex, so the mutex is at the head of its own block of things it protects) https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:108: func Dial(network, raddr string, priority Priority, tag string) (w *Writer, err error) { drop the named result parameters here. they add nothing to the docs. just: (*Writer, error) https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:118: w = &Writer{priority: priority, tag: tag, hostname: hostname, conn: nil, network: network, raddr: raddr} go multiple lines here, and you can drop the "conn: nil", or if you really want it there for explictness, at least explain it: conn: nil, // initialized by connect, below https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:121: err = w.connect() err := https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:125: return return w, nil https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:130: func (w *Writer) connect() (err error) { this named result parameter is okay, btw, since it's a private method and doesn't pollute godoc. https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:160: func (w *Writer) Close() (err error) { drop the named result parameter.
Sign in to reply to this message.
Hello mikioh.mikioh@gmail.com, rsc@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM On Fri, Dec 14, 2012 at 1:08 AM, <jeff.allen@gmail.com> wrote: > Hello mikioh.mikioh@gmail.com, rsc@golang.org, bradfitz@golang.org (cc: > > golang-dev@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.**com/6782140/<https://codereview.appspot.com/6782... >
Sign in to reply to this message.
Leaving for Mikioh to also review and then submit. On Fri, Dec 14, 2012 at 9:02 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > LGTM > > On Fri, Dec 14, 2012 at 1:08 AM, <jeff.allen@gmail.com> wrote: > >> Hello mikioh.mikioh@gmail.com, rsc@golang.org, bradfitz@golang.org (cc: >> >> golang-dev@googlegroups.com), >> >> Please take another look. >> >> >> https://codereview.appspot.**com/6782140/<https://codereview.appspot.com/6782... >> > >
Sign in to reply to this message.
Please revisit test cases because I've just modified ListenUnixgram signature. Also please provide multiple fake syslog servers which use "tcp", "udp", "unix" and "unixgram". And it would be better not only concurrent write test but concurrent re-connect test. Thanks. https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:89: type serverConn interface { I think this interface is unnecessary unless you want to use both net.Conn and net.PacketConn. https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:94: type netConn struct { ditto. https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:167: func (w *Writer) Close() error { Close is exposed so it needs comments. https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:239: if w.conn == nil { I'm not keen to manage a conn state with write-only lock because the state of conn might be a single point of failure to all syslog.Writer users in a single go process. For example, once the conn brings down, all users would be blocked by this state until new conn estab attempts finished (no DialTimeout...). I have no clue, do you have any good solution for this? E.g., anyway syslog.Writer fails as quick as possible and wont block up users during re-connecting.
Sign in to reply to this message.
Most comments have been addressed. Still need to do the concurrent reconnect test. https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:82: mu sync.Mutex On 2012/12/13 20:32:36, bradfitz wrote: > either put a space before this line and/or add a comment on the same line after > sync.Mutex saying what it guards: > > mu sync.Mutex // guards conn, network, raddr > > or > > mu sync.Mutex // guards conn > > (in which case network and raddr should go above the mutex, so the mutex is at > the head of its own block of things it protects) Done. https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:108: func Dial(network, raddr string, priority Priority, tag string) (w *Writer, err error) { On 2012/12/13 20:32:36, bradfitz wrote: > drop the named result parameters here. they add nothing to the docs. just: > > (*Writer, error) Done. https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:118: w = &Writer{priority: priority, tag: tag, hostname: hostname, conn: nil, network: network, raddr: raddr} On 2012/12/13 20:32:36, bradfitz wrote: > go multiple lines here, and you can drop the "conn: nil", or if you really want > it there for explictness, at least explain it: > > conn: nil, // initialized by connect, below Done. https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:121: err = w.connect() On 2012/12/13 20:32:36, bradfitz wrote: > err := Done. https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:125: return On 2012/12/13 20:32:36, bradfitz wrote: > return w, nil Done. https://codereview.appspot.com/6782140/diff/38001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:160: func (w *Writer) Close() (err error) { On 2012/12/13 20:32:36, bradfitz wrote: > drop the named result parameter. Done. https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:89: type serverConn interface { On 2012/12/16 06:54:17, mikio wrote: > I think this interface is unnecessary unless you want to > use both net.Conn and net.PacketConn. Done. https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:94: type netConn struct { On 2012/12/16 06:54:17, mikio wrote: > ditto. Done. https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:167: func (w *Writer) Close() error { On 2012/12/16 06:54:17, mikio wrote: > Close is exposed so it needs comments. Done. https://codereview.appspot.com/6782140/diff/42002/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:239: if w.conn == nil { I think the correct approach to this risk is to reduce the number of callers to syslog.Write, and the correct way to do that is for the calling application to queue pending log message itself via a buffering channel. I think the vast majority of callers don't need this, so I don't think it should be in the standard library. (For comparison: log.New on an io.Writer which represents a TCP pipe could cause log.Write to block while holding log.Logger.mu, and all other callers would block as well.) So my proposal is to not do anything and let the caller deal with it if they care. I could add a sentence or two in the docs explaining this.
Sign in to reply to this message.
> I think the correct approach to this risk is to reduce the number of callers to > syslog.Write, and the correct way to do that is for the calling application to > queue pending log message itself via a buffering channel. I think the vast > majority of callers don't need this, so I don't think it should be in the > standard library. (snip) > So my proposal is to not do anything and let the caller deal with it if they > care. I could add a sentence or two in the docs explaining this. fair enough. > (For comparison: log.New on an io.Writer which represents a > TCP pipe could cause log.Write to block while holding log.Logger.mu, and all > other callers would block as well.) there's a difference btw log and syslog. log transfers all connection setup/teardown controls to users but syslog takes almost all. for example, your CL allows to revive a closed syslog.Writer by using syslog.Write. is this intentional behavior?
Sign in to reply to this message.
On 2012/12/19 00:30:55, mikio wrote: > for example, your CL allows to revive a closed syslog.Writer by using > syslog.Write. is this intentional behavior? Yes. I see it as a not-unuseful side effect of the very useful fix to the legitimate issue 2264. A caller who does not want a given *Writer to reconnect should lose its last reference to it, and it will be GC'd.
Sign in to reply to this message.
Please take another look. The concurrent test is done now, and the mystery about Read vs. ReadFrom is solved (see issue 4636). This CL is now depending on issue 4636 to be fixed (CL submitted for it).
Sign in to reply to this message.
This CL has just been updated to work with tip of tree. Please check it in when issue 4636 is fixed.
Sign in to reply to this message.
https://codereview.appspot.com/6782140/diff/55001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/55001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:127: // (*Writer).connect makes a connection to the syslog server. you can just say "connect". // connect makes ... https://codereview.appspot.com/6782140/diff/55001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:154: // Write sends a log message to the syslog daemon. A failing this is a lot of words. I think a subset of this could be moved to the package comment instead, since it applies to all methods, not just Write.
Sign in to reply to this message.
https://codereview.appspot.com/6782140/diff/55001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/55001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:119: // w.mu not held, but w is still private to us at this point w.mu.Lock() defer w.mu.Unlock() connect says it requires w.mu to be held. make that true. much better than a comment. having one reference is not the same as holding a lock. what if connect kicks off a goroutine that will do something with w after acquiring the lock, under the assumption that the 'something' can't happen until connect returns? the lock isn't actually held in this call so the call to connect and the hypothetical goroutine will step on each other. https://codereview.appspot.com/6782140/diff/55001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:238: if w.conn == nil { Doesn't this end up connecting twice? Simpler: if w.conn != nil { if n, err := w.write(pr, s); err == nil { return n, err } } if err := w.connect(); err != nil { return 0, err } return w.write(pr, s) https://codereview.appspot.com/6782140/diff/55001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:265: _, isTcp := w.conn.(*net.TCPConn) This has nothing to do with reconnecting and should not be in this CL. I would probably argue it shouldn't be done anyway, but it definitely shouldn't be in this CL.
Sign in to reply to this message.
Something has to be done about embedded newlines, because they are the record separator for stream-oriented transports. An embedded newline causes the remainder of the message to be lost. If you only talk syslog over unixgram or UDP, the datagram is the record, so you don't need to worry about embedded newlines.
Sign in to reply to this message.
Hello mikioh.mikioh@gmail.com, rsc@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Seems fine, leaving for Brad and Mikio. We can have a discussion about newlines in messages, just not in a CL about redialing. https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... File src/pkg/log/syslog/syslog_test.go (right): https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:54: } else { no else after return golang.org/doc/effective_go.html#if https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:58: for { for ct := 1; !crashy || ct%7 != 0; ct++ { https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:61: if crashy && (ct%7) == 0 { drop unnecessary () https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:71: return unneeded; delete https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:171: s, err := New(LOG_INFO|LOG_USER, "tcp:127.0.0.1:514") Why this address-looking string as a tag? https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:173: t.Fatalf("New() failed: %v", err) FWIW %s was fine. https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:203: t.Fatalf("Dial() failed: %v", err) FWIW %s was fine. https://codereview.appspot.com/6782140/diff/58003/src/pkg/log/syslog/syslog_t... src/pkg/log/syslog/syslog_test.go:219: return No need for return at end of function that doesn't return anything. Delete.
Sign in to reply to this message.
Hello mikioh.mikioh@gmail.com, rsc@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:11: // A failing write will cause the package to try to reconnect one time. The "one time" is correct "per-write", but it reads like you get one reconnect only, ever. I would also drop the second sentence justifying it. If you want that text, move it to the retry function itself, but I doubt end-users care. And if the intent is to calm users into thinking their syslogged message will make it through, you don't explicitly say that the reconnect is before the re-write of the message that failed. Maybe something like: // Only one call to Dial is necessary. On write failures // the syslog client will attempt to reconnect to the server // and write again. https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:166: func (w *Writer) Close() error { is this meant to be permanent? the next write will just re-dial again, right? https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:251: // write: generates and writes a syslog formatted string. The drop the colon https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:254: // If transport is stream-based, pay attention that the only \n is at this comment no longer belongs, right? this will be in a future CL? https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:259: if len(msg) == 0 || msg[len(msg)-1] != '\n' { if !strings.HasSuffix(msg, "\n") { ?
Sign in to reply to this message.
brad, PTAL. https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.go File src/pkg/log/syslog/syslog.go (right): https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:166: func (w *Writer) Close() error { It is for compatibility with the previous api, and to let someone who it counting every last file descriptor free the one to the syslog daemon, I guess. https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:251: // write: generates and writes a syslog formatted string. The On 2013/02/01 17:08:10, bradfitz wrote: > drop the colon Done. https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:254: // If transport is stream-based, pay attention that the only \n is at On 2013/02/01 17:08:10, bradfitz wrote: > this comment no longer belongs, right? this will be in a future CL? Done. https://codereview.appspot.com/6782140/diff/68001/src/pkg/log/syslog/syslog.g... src/pkg/log/syslog/syslog.go:259: if len(msg) == 0 || msg[len(msg)-1] != '\n' { On 2013/02/01 17:08:10, bradfitz wrote: > if !strings.HasSuffix(msg, "\n") { > > ? Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=8d71734a0cb0 *** log/syslog: retry once if write fails Implements deferred connections + single-attempt automatic retry. Based on CL 5078042 from kuroneko. Fixes issue 2264. R=mikioh.mikioh, rsc, bradfitz CC=golang-dev https://codereview.appspot.com/6782140 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
