Skip to content
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

Use 503 for "soft" failures. #1044

Merged
merged 4 commits into from
Feb 10, 2023
Merged

Use 503 for "soft" failures. #1044

merged 4 commits into from
Feb 10, 2023

Conversation

bgrozev
Copy link
Member

@bgrozev bgrozev commented Feb 9, 2023

  • ref: Move JicofoHealthChecker to kotlin.
  • feat: Adapt to jicoco changes, use "soft" failure (503 instead of 500) for MUC and ping timeouts.

}

private fun performCheck() {
Objects.requireNonNull(focusManager, "FocusManager is not set.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be requireNonNull in Kotlin.

check()
val duration = System.currentTimeMillis() - start
if (duration > HealthConfig.config.maxCheckDuration.toMillis()) {
logger.error("Health check took too long: " + duration + "ms")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use interpolation

if (bridgeSelector.operationalBridgeCount <= 0) {
throw RuntimeException(
"No operational bridges available (total bridge count: "
+ BridgeSelector.bridgeCount.get() + ")")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use interpolation.

throw RuntimeException("Failed to create conference with room name $roomName")
}
} catch (e: Exception) {
throw RuntimeException("Failed to create conference with room name " + roomName + ":" + e.message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use interpolation.


// Wait for 5 seconds to receive a response.
if (!pingResponseWait.await(5, TimeUnit.SECONDS)) {
throw RuntimeException("did not receive xmpp ping response for (${xmppProvider.config.name})")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the name in parentheses?

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #1044 (b56b193) into master (7aa42db) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1044      +/-   ##
============================================
- Coverage     27.42%   27.33%   -0.10%     
  Complexity      477      477              
============================================
  Files           124      124              
  Lines          7416     7437      +21     
  Branches       1056     1061       +5     
============================================
- Hits           2034     2033       -1     
- Misses         5130     5149      +19     
- Partials        252      255       +3     
Impacted Files Coverage Δ
...ava/org/jitsi/jicofo/health/JicofoHealthChecker.kt 0.00% <0.00%> (ø)
...src/main/kotlin/org/jitsi/jicofo/JicofoServices.kt 1.43% <0.00%> (-0.06%) ⬇️
...ava/org/jitsi/jicofo/codec/JingleOfferFactory.java 89.13% <0.00%> (-8.49%) ⬇️
...tlin/org/jitsi/jicofo/conference/ConferenceUtil.kt 52.17% <0.00%> (-4.35%) ⬇️
.../main/kotlin/org/jitsi/jicofo/codec/CodecConfig.kt 0.00% <0.00%> (ø)
...org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aa42db...b56b193. Read the comment docs.

@bgrozev bgrozev merged commit d5fcce9 into master Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants