-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add handling for tcp register timeout leaving connection dead #1183
Conversation
d1c48f3
to
0cecc72
Compare
0cecc72
to
4be646b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, @pjfanning @Roiocam @He-Pin @jxnu-liguobin @raboof Should also take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
maybe in some further iteration, we could look at retries - but this is an improvement as is |
LGTM |
I was thinking going the other direction and questioning the need for the So do we need to have that capability to have a different handler? Or could we inline everything into the connection open and avoid having that timeout entirely? It also seems to be a really really rare case since it usually only takes milliseconds (receiving the message and replying back). We only ever saw that in case of a test environment we use in low power mode and deploy a complete system to it. |
actor/src/main/scala/org/apache/pekko/io/dns/internal/TcpDnsClient.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may need to investigate deeper. ❤️
actor/src/main/scala/org/apache/pekko/io/dns/internal/TcpDnsClient.scala
Show resolved
Hide resolved
I will TAL at weekend, a little busy at $Work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, as it was supervised by org.apache.pekko.pattern.BackoffSupervisor
.
Got enough approvals, merging so it can get released with M1 |
Refs #1182