Added PeerManagement.Timeout config option #491
Conversation
thrawn01
force-pushed
the
thrawn.peer-timeout
branch
from
5fb061f to
ef32214
Compare
JamieDanielson
added
the
status: oncall
thrawn01
force-pushed
the
thrawn.peer-timeout
branch
from
ef32214 to
128f0d6
Compare
thrawn01
force-pushed
the
thrawn.peer-timeout
branch
from
128f0d6 to
3b3137e
Compare
internal/peer/redis.go
Outdated
| if err != nil { | ||
| // TODO maybe do something better here? | ||
| logrus.WithField("name", p.publicAddr). |
There was a problem hiding this comment.
A nit, but could you please do this with logrus.WithFields rather than a succession of WithField calls?
There was a problem hiding this comment.
@thrawn01 -- to clarify, although I approved this PR, I would like to see that change before we merge it. Are you able to do that, please?
| @@ -1,3 +1,4 @@ | |||
| //go:build all || race | |||
There was a problem hiding this comment.
We're on 1.18+, so this is a good change. Thank you.
There was a problem hiding this comment.
As per @kentquirk's request, please can you update your logs to use WithFields 👍🏻
MikeGoldsmith
added
type: enhancement
|
This will also need a rebase to resolve conflicts. |
Done. |
|
@Baliedge please could you update the log to use WithFields as @kentquirk suggestion too? |
Also done. |
Co-authored-by: Shawn Poulson <[email protected]>
Which problem is this PR solving?
This PR adds a new config option called
PeerManagement.Timeoutwhich will be used when communicating with peer discovery systems implementing thePeersinterface. This PR also add support forcontext.Contextwhen first connecting to thePeersimplementation.Short description of the changes
NewPeers()now accepts acontext.ContextNewPeers()initialization after logging setup to take advantage of logs during any initial connect attempts.//go:build all || racewhich is the new 1.18+ format for build tags (I can revert if not needed, my IDE auto fixed it 😄 )Routerwhere thecancel()function is never called.Note
I'm noting here that
context.Contextpassed toNewPeer()is not as useful to redis because the redis dial function already has a retry and timeout loop. However, it will be useful for other concrete implementations of thePeersinterface. If this PR is accepted, I wish to follow up with a PR that uses https://github.com/hashicorp/memberlist for peer discovery.