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

Feat/openmedia hotstandby heartbeat logic #105

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

olzzon
Copy link
Contributor

@olzzon olzzon commented Oct 8, 2024

About the Contributor

This is a PR made on behalf of BBC

Type of Contribution

This is a Feature

Current Behavior

Currently MOS-connection checks for heartbeat on all devices.

New Behavior

There has been added an option openMediaHotStandby?: boolean in the API
When set to true, the heartbeat will be enabled/disabled depending on what OpenMedia server is being used.

Testing Instructions

For test a dual OpenMedia server setup is needed, and openMediaHotStandby must be set on the secondary connection in deviceOptions.

deviceOptions.secondary.openMediaHotStandby = true
const mosDevice: MosDevice = await this.mos.connect(deviceOptions)

Other Information

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

@olzzon olzzon marked this pull request as ready for review October 17, 2024 09:46
@olzzon
Copy link
Contributor Author

olzzon commented Oct 17, 2024

Converted the PR to upstream from Draft to Ready be reviewed/merged

@nytamin nytamin self-requested a review October 21, 2024 05:20
Copy link
Contributor

@nytamin nytamin left a comment

Choose a reason for hiding this comment

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

I'm missing a few things in this one to be able to merge it.

Mainly I'm missing unit tests covering this situation, and the mosDevice.getConnectionStatus() is not updated/defined how it should work.

I've added a (failing) unit test in the branch chore/pr-105 to highlight some of the issues, please have a look at it and feel free merge it into your branch.

Also I have a question regarding the OpenMedia server: When the secondary server is standing by, does it accept any Socket connections still, or is it completely offline? (This is relevant to designing the unit test properly).

packages/connector/src/MosConnection.ts Outdated Show resolved Hide resolved
@olzzon olzzon marked this pull request as draft October 21, 2024 06:41
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.87%. Comparing base (a348b97) to head (562d74b).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   75.28%   75.87%   +0.59%     
==========================================
  Files          67       67              
  Lines        3985     4009      +24     
  Branches      949      948       -1     
==========================================
+ Hits         3000     3042      +42     
- Misses        906      943      +37     
+ Partials       79       24      -55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@olzzon olzzon marked this pull request as ready for review October 28, 2024 09:41
@nytamin nytamin self-requested a review October 28, 2024 09:59
@mint-dewit
Copy link
Contributor

I've added some commits to this PR to better reflect what this feature entails. Specifically clarifying that the connection status report for the individual connections should still reflect the actual status of the system.

I am open to adding a method that exposes a single status for both primary and secondary connection but if we are reporting the individual connections we should not add a "fake" connected status. That smells of bad magic. The user of the lib is already aware of the system being a "hot standby" and should interpret the status of the secondary accordingly.

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.

4 participants