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
Allow to enable pings for specific remote clusters #34753
Changes from 6 commits
e016ae7
edfdac5
95b55f3
19b4bac
239ddc1
fae393a
ce98370
dafcabf
74af6d7
4df073d
fd43e39
8550bad
a0ca97c
3e984ac
8b27267
076aef8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,9 +46,9 @@ between all nodes. Defaults to `false`. | |
|
||
|`transport.ping_schedule` | Schedule a regular application-level ping message | ||
to ensure that transport connections between nodes are kept alive. Defaults to | ||
`5s` in the transport client and `-1` (disabled) elsewhere. It is preferable to | ||
correctly configure TCP keep-alives instead of using this feature, because TCP | ||
keep-alives apply to all kinds of long-lived connection and not just to | ||
`5s` in the transport client and `-1` (disabled) elsewhere. It is preferable | ||
to correctly configure TCP keep-alives instead of using this feature, because | ||
TCP keep-alives apply to all kinds of long-lived connections and not just to | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debates rage to this day on the internet about the use of singular or plural after "kinds of". I suspect that British English prefers the singular and US English the plural, and both are ok 😄 🇬🇧 (I'm ok with this change, just thought you'd like to know) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. interesting :) |
||
transport connections. | ||
|
||
|======================================================================= | ||
|
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 need to say that this setting sets the time between pings, otherwise it's not clear what values other than
-1
mean. I also think that the "defaults to ... which defaults to ..." in the last sentence might cause confusion. I drafted an alternative:I'm not sure this is correct, however. If we set
transport.ping_schedule: 5s
cluster.remote.foo.transport.ping_schedule: -1
Does this disable pings to the
foo
remote? Should it? I think it'd be useful to be able to do so. I haven't dug into the implementation but there's no test for this case as far as I can see.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.
it sounds great, I was hoping you would help out rephrasing the docs, thanks a lot for that. The behaviour should be what you describe with transport.ping_schedule as a fallback, but I will add a test for this specific case that you mention, it's a good point.