Add transcode queue insights into health check and integrate with service selection #1758
Conversation
dmanjunath
changed the title
libs/src/utils/network.js
Outdated
|
|
||
| // Sort by content node transcode queue load | ||
| // defined as the ratio of (active + waiting transcodes) / (transcode slots, same as number of cpu cores) | ||
| const healthRatioA = ((a.response.data.data.transcodeActive + a.response.data.data.transcodeWaiting) / a.response.data.data.numberOfCPUs) |
There was a problem hiding this comment.
wait this method is shared isn't it? should we be checking on whether these fields exist?
There was a problem hiding this comment.
also for backwards compatibility, this would throw if these fields don't exist right? we can't release this until all cnodes update. i think the libs PR should be separated from the cnode PR in that case
There was a problem hiding this comment.
if one/all of these fields don't exist or are 0, as far as the sort is concerned it does nothing so it should pass through to the time tiebreaker
also it doesn't error if these fields don't exist since pulling a nonexistent property off an object is undefined. it's only when you pull a property off another property that doesn't exist it errors. but i can add validation to make it cleaner
There was a problem hiding this comment.
const healthRatioA = ((a.response.data.data.transcodeActive + a.response.data.data.transcodeWaiting) / a.response.data.data.numberOfCPUs)
this would be undefined / some value
won't that throw?
There was a problem hiding this comment.
it actually doesn't, it becomes NaN, but let me just add some checking. i'm also happy to separate but I don't think we need to. i think just defaulting the ratio to 1 to simulate all transcode slots taken should be sufficient. seems like the easiest way to ensure backwards/forwards compat
in the case that a node upgrades, we'll get the real data and since it's likely to be lower than 1 it'll be selected.
in the case everyone upgrades, we'll have accurate data
in the case nobody upgrades, it'll be the same for everyone and the tie breaker will be time
There was a problem hiding this comment.
default sounds reasaonble
| @@ -19,7 +19,7 @@ const MIN_FILESYSTEM_SIZE = 1950000000000 // 1950 GB of file system storage | |||
| * @param {string?} randomBytesToSign optional bytes string to be included in response object | |||
| * and used in signature generation | |||
| */ | |||
| const healthCheck = async ({ libs } = {}, logger, sequelize, getMonitors, numberOfCPUs, randomBytesToSign = null) => { | |||
| const healthCheck = async ({ libs } = {}, logger, sequelize, getMonitors, numberOfCPUs, getTranscodeQueueJobs, randomBytesToSign = null) => { | |||
There was a problem hiding this comment.
i assume you meant to change healthCheckController call to this function and set this param to false? either way i think we should just revert this change
There was a problem hiding this comment.
ah since you never consume it i guess you actualy meant to revert this
There was a problem hiding this comment.
oh yeah leftover relic, will remove
libs/src/utils/network.js
Outdated
| // If same version, do a tie breaker on the response time | ||
|
|
||
| // Sort by content node transcode queue load | ||
| // defined as the ratio of (active + waiting transcodes) / (transcode slots, same as number of cpu cores) |
libs/src/utils/network.js
Outdated
| } | ||
|
|
||
| // lower is better, means less transcode queue utilization | ||
| // default ratio for transcode count and num cpus if node doesn't have one of those defined is 1 |
There was a problem hiding this comment.
i think this leads to unintended behavior if one or both nodes haven't defined transcode counts
CN1 = 8 numCPUs
CN2 = 4 numCPUs -> 20 transcodeCount
outcome: 8/8 = 1 & 20/4 = 5; 1 < 5; CN1 preferred. that seems incorrect? what if CN1 has 50 transcodeCount
feels like we should only consume transcodeCounts if both nodes provide them, else only use numCPUs which we know all nodes provide
There was a problem hiding this comment.
my rationale was if one reports transcode count and the other doesnt, it's likely behind in version so the higher version will get placed ahead anyway. this case should only really get hit if both nodes don't have something
| @@ -143,6 +143,9 @@ const healthCheckVerbose = async ({ libs, snapbackSM } = {}, logger, sequelize, | |||
| currentSnapbackReconfigMode = snapbackSM.highestEnabledReconfigMode | |||
| } | |||
|
|
|||
| const { active: transcodeActive, waiting: transcodeWaiting } = await getTranscodeQueueJobs() | |||
| console.log('transcodeActive', transcodeActive, transcodeWaiting) | |||
| } | ||
| console.log(response.transcodeActive, response.transcodeWaiting, 'respszz') |
|
looks like some test code so will hold off reviewing ¨ntil you re-request |
Description
Add criteria in content node selection to sort healthy nodes by transcode queue utilization to better balance load across different content nodes
Tests
Added unit tests, testing manually now.
How will this change be monitored?
Amplitude has creator node selection graphs