ExternalBridgeSelectionStrategy #889
Conversation
|
Hi, thanks for your contribution! |
|
Well, I guess the existence of CI testing Java 8 tells me that using |
|
We've been deploying jicofo on Java 11 AFAIK. Maybe we ought to update the CI checks? /cc @bgrozev ? |
jbg
force-pushed
the
jbg/external-bridge-selection-strategy
branch
from
7624ad1
to
4c5d544
Compare
jbg
force-pushed
the
jbg/external-bridge-selection-strategy
branch
from
4c5d544
to
bc8ab65
Compare
|
Thanks, that's definitely a good contribution! I'll check back with the team to see if there's anything preventing us from dropping support for java 8. |
|
Maybe Ubuntu 18.04 ... but that is 4 years old now ... |
jbg
changed the title
|
We're working on dropping java8 support, and will review/merge when that's done. |
| .connectTimeout(ExternalBridgeSelectionStrategyConfig.config.timeout) | ||
| .build() | ||
|
|
||
| private val logger: Logger = LoggerImpl(ExternalBridgeSelectionStrategy::class.simpleName) |
There was a problem hiding this comment.
You can use createLogger() here
| class ExternalBridgeSelectionStrategy() : BridgeSelectionStrategy() { | ||
| private val httpClient: HttpClient = HttpClient | ||
| .newBuilder() | ||
| .connectTimeout(ExternalBridgeSelectionStrategyConfig.config.timeout) |
There was a problem hiding this comment.
You can import ExternalBridgeSelectionStrategyConfig.config to make these calls shorter
| ?: throw Exception("ExternalBridgeSelectionStrategy requires url to be provided") | ||
|
|
||
| val requestBody = JSONObject() | ||
| requestBody["bridges"] = bridges?.map { |
There was a problem hiding this comment.
I just realized that you are providing the full list of bridges here. Can you make that optional (behind a config flag) please? We intend to use it with a service which already has the list of available bridges.
There was a problem hiding this comment.
It's necessary as long as the index is used in the response to identify the bridge. But if the JID is used as suggested, this can even be removed entirely rather than made optional IMO (our service also knows the list of available bridges).
But I was concerned how Jicofo would react if told to use a bridge it doesn't already know about. Even if not deliberate, I can imagine when bridges start up or shut down there could be brief periods where the external selection service has a different idea of available bridges than Jicofo.
| } | ||
| requestBody["conference_bridges"] = conferenceBridges?.mapKeys { it.key.jid.toString() } | ||
| requestBody["participant_region"] = participantRegion | ||
| requestBody["fallback_strategy"] = ExternalBridgeSelectionStrategyConfig.config.fallbackStrategy |
There was a problem hiding this comment.
We were using it to signal which mode the external service should use (region based or intra region), matched to the fallback strategy Jicofo would use if it couldn't contact the external service for any reason. We're actually now passing the selected mode in the URL, so this is no longer needed. I'll remove it, absent any use case for it.
| try { | ||
| response = httpClient.send(request, HttpResponse.BodyHandlers.ofString()) | ||
| } catch (exc: Exception) { | ||
| logger.error("ExternalBridgeSelectionStrategy: HTTP request failed with ${exc}, using fallback strategy") |
There was a problem hiding this comment.
Remove "ExternalBridgeSelectionStrategy" from the message, it's already in the logger context.
| return fallback(bridges, conferenceBridges, participantRegion) | ||
| } | ||
|
|
||
| val bridge = bridges!![selectedBridgeIndex.toInt()] |
There was a problem hiding this comment.
Can you fail early (before sending the request) if bridges is null or empty?
| return fallback(bridges, conferenceBridges, participantRegion) | ||
| } | ||
|
|
||
| val bridge = bridges!![selectedBridgeIndex.toInt()] |
There was a problem hiding this comment.
What do you think about having the external service return an ID (JID) of the selected bridge instead of an index? We want to have the service return a bridge that jicofo doesn't yet know about (this will require more changes, but it will be easier to have the HTTP API ready to support it and not have to change).
There was a problem hiding this comment.
Yes, this seems sensible. Interesting use case to use a bridge it doesn't know about yet. Launching bridges on-demand?
| .POST(HttpRequest.BodyPublishers.ofString(requestBody.toJSONString())) | ||
| .uri(URI.create(url)) | ||
| .headers("Content-Type", "application/json") | ||
| .timeout(ExternalBridgeSelectionStrategyConfig.config.timeout) |
There was a problem hiding this comment.
Will this work as expected when the timeout is null? You could either only call .timeout if the config has a value, or make the config value required (by config instead of by optionalconfig) and add a reasonable default value in reference.conf
There was a problem hiding this comment.
Good catch, .timeout(null) will throw. I think making it required with a default in reference.conf is sensible; there is no rational reason to want to block forever...
| } | ||
| fun timeout() = timeout | ||
|
|
||
| val fallbackStrategy: String? by optionalconfig { |
There was a problem hiding this comment.
Perhaps it's better to make this required (by config) and provide a good default value?
|
One more thing: can you please add a commented out section to |
|
@bgrozev Thanks for the review and sorry for the long delay before getting back to this. I'll rebase and address your comments shortly. |
This is very similar to a patch we've been using at AVStack for a while, and I think it could be useful for others.
Basically our bridge selection logic is a bit more complex than we'd like to express inside Jicofo itself, so we have Jicofo call an external (to Jicofo) service for bridge selection decisions. A fallback to another strategy is implemented in case it's unable to reach the service.
This lets us look up the customer's bridge preferences in a database (allowing customers to set up rules about which regions are allowed or denied themselves), as well as implement failover to nearby regions etc.
I've marked this as RFC because I'm not sure if it's something you want included in Jicofo itself, and also because the use of
HttpClientenforces a Java 11 minimum. I'm not sure if there is a published policy about minimum Java version; if we still need to support older than 11 then we could swap out the use ofHttpClientfor something else.