-
Notifications
You must be signed in to change notification settings - Fork 5
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 http timeout configuration for nodes #22
Conversation
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.
Looks good at first glance, just a few changes I would recommend making
*/ | ||
@JvmOverloads | ||
fun addNode(name: String, address: URI, password: String, regionFilter: IRegionFilter? = null): LavalinkNode { | ||
fun addNode(name: String, address: URI, password: String, regionFilter: IRegionFilter? = null, httpTimeout: Long = TIMEOUT_MS): LavalinkNode { |
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.
With how many parameters there currently are, it may be better to deprecate this method and create a method that accepts a builder instead.
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 was thinking a builder or some sort of node options class that can be extended with all of these
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.
That sounds good to me. Something like new NodeOptions.Builder().build()
-> NodeOptions
Added the LavalinkNodeOptions builder, can add some javadocs if you want, but left em out for now because they seem kinda straightforward |
src/main/kotlin/dev/arbjerg/lavalink/client/LavalinkNodeOptions.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/dev/arbjerg/lavalink/client/LavalinkNodeOptions.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/dev/arbjerg/lavalink/client/LavalinkNodeOptions.kt
Outdated
Show resolved
Hide resolved
…use new NodeOptions builder
c15d013
to
3d6e53d
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
Currently nodes only use the default HTTP timeouts set by OKHTTP, which are:
This PR allows the consumer to set an optional total call timeout. It utilizes the currently unused constant TIMEOUT_MS and changes that to a default of 10sec per request. 10 seconds was chosen to avoid moving the timeout up too much from what it is now, 10 sec to connect, 10 more sec to finish read/write op. I doubt many users have latency > 5s but it avoids someone from getting a regression if they do.