-
-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Fix #6586: Reset retries and set UP status for successful push heartb… #6647
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
Draft
tellorian
wants to merge
12
commits into
louislam:master
Choose a base branch
from
tellorian:fix/issue-6586-push-monitor-retries
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+138
−56
Draft
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
4425a2b
Fix #6586: Reset retries and set UP status for successful push heartb…
tellorian 5262bfc
Merge branch 'master' into fix/issue-6586-push-monitor-retries
tellorian f1dba1f
Fix #6586: Reset retries when PENDING status exceeds maxretries in pu…
tellorian 645d313
Clean up test file following best practices
tellorian dc2b6f9
[autofix.ci] apply automated fixes
autofix-ci[bot] 548069d
Merge branch 'master' into fix/issue-6586-push-monitor-retries
tellorian 14c6f56
Merge branch 'master' into fix/issue-6586-push-monitor-retries
tellorian 0722b99
[autofix.ci] apply automated fixes
autofix-ci[bot] 3811433
Merge branch 'master' into fix/issue-6586-push-monitor-retries
CommanderStorm 41fe102
Merge branch 'master' into fix/issue-6586-push-monitor-retries
tellorian fc9e223
Refactor determineStatus function and improve test coverage
tellorian efe98a6
[autofix.ci] apply automated fixes
autofix-ci[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| const { UP, DOWN, PENDING, flipStatus } = require("../src/util"); | ||
|
|
||
| /** | ||
| * Determine the status of a monitor based on the current status, previous heartbeat, and configuration | ||
| * @param {number} status - The current status from the monitor | ||
| * @param {object} previousHeartbeat - The previous heartbeat object | ||
| * @param {number} maxretries - The maximum number of retries allowed | ||
| * @param {boolean} isUpsideDown - Indicates if the monitor is upside down | ||
| * @param {object} bean - The new heartbeat object | ||
| * @returns {void} | ||
| */ | ||
| function determineStatus(status, previousHeartbeat, maxretries, isUpsideDown, bean) { | ||
| if (isUpsideDown) { | ||
| status = flipStatus(status); | ||
| } | ||
|
|
||
| if (previousHeartbeat) { | ||
| if (previousHeartbeat.status === UP && status === DOWN) { | ||
| // Going Down | ||
| if (maxretries > 0 && previousHeartbeat.retries < maxretries) { | ||
| // Retries available | ||
| bean.retries = previousHeartbeat.retries + 1; | ||
| bean.status = PENDING; | ||
| } else { | ||
| // No more retries | ||
| bean.retries = 0; | ||
| bean.status = DOWN; | ||
| } | ||
| } else if (previousHeartbeat.status === PENDING && status === DOWN) { | ||
| if (previousHeartbeat.retries < maxretries) { | ||
| // Retries available | ||
| bean.retries = previousHeartbeat.retries + 1; | ||
| bean.status = PENDING; | ||
| } else { | ||
| // No more retries, set to DOWN and reset retries | ||
| bean.retries = 0; | ||
| bean.status = DOWN; | ||
| } | ||
| } else { | ||
| // No more retries or not pending | ||
| if (status === DOWN) { | ||
| bean.retries = previousHeartbeat.retries + 1; | ||
| bean.status = status; | ||
| } else { | ||
| bean.retries = 0; | ||
| bean.status = status; | ||
| } | ||
| } | ||
| } else { | ||
| // First beat? | ||
| if (status === DOWN && maxretries > 0) { | ||
| // Retries available | ||
| bean.retries = 1; | ||
| bean.status = PENDING; | ||
| } else { | ||
| // Retires not enabled | ||
| bean.retries = 0; | ||
| bean.status = status; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| module.exports = { | ||
| determineStatus, | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as said: please rename the test Also, please see the warnings in the files changed tab |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| const { describe, test } = require("node:test"); | ||
| const assert = require("node:assert"); | ||
| const { UP, DOWN, PENDING } = require("../../src/util"); | ||
| const { determineStatus } = require("../../server/determine-status"); | ||
|
|
||
| describe("Push API Retry Reset Issue #6586", () => { | ||
CommanderStorm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| test("determineStatus() resets retries to 0 when status changes from DOWN to UP", () => { | ||
| const previousHeartbeat = { | ||
| status: DOWN, | ||
| retries: 2, | ||
| }; | ||
|
|
||
| const bean = {}; | ||
|
|
||
| determineStatus(UP, previousHeartbeat, 3, false, bean); | ||
|
|
||
| assert.strictEqual(bean.status, UP); | ||
| assert.strictEqual(bean.retries, 0); | ||
| }); | ||
|
|
||
| test("determineStatus() resets retries to 0 when status changes from PENDING to UP", () => { | ||
| const previousHeartbeat = { | ||
| status: PENDING, | ||
| retries: 1, | ||
| }; | ||
|
|
||
| const bean = {}; | ||
|
|
||
| determineStatus(UP, previousHeartbeat, 3, false, bean); | ||
|
|
||
| assert.strictEqual(bean.status, UP); | ||
| assert.strictEqual(bean.retries, 0); | ||
| }); | ||
|
|
||
| test("determineStatus() handles PENDING status exceeding maxretries correctly", () => { | ||
| const previousHeartbeat = { | ||
| status: PENDING, | ||
| retries: 3, | ||
| }; | ||
|
|
||
| const bean = {}; | ||
|
|
||
| determineStatus(DOWN, previousHeartbeat, 3, false, bean); | ||
|
|
||
| assert.strictEqual(bean.status, DOWN); | ||
| assert.strictEqual(bean.retries, 0); | ||
| }); | ||
|
|
||
| test("determineStatus() increments retries when status remains DOWN", () => { | ||
| const previousHeartbeat = { | ||
| status: DOWN, | ||
| retries: 1, | ||
| }; | ||
|
|
||
| const bean = {}; | ||
|
|
||
| determineStatus(DOWN, previousHeartbeat, 3, false, bean); | ||
|
|
||
| assert.strictEqual(bean.status, DOWN); | ||
| assert.strictEqual(bean.retries, 2); | ||
| }); | ||
|
|
||
| test("determineStatus() handles first heartbeat correctly", () => { | ||
| const bean = {}; | ||
|
|
||
| determineStatus(UP, null, 3, false, bean); | ||
|
|
||
| assert.strictEqual(bean.status, UP); | ||
| assert.strictEqual(bean.retries, 0); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
please explain why you did not just import from the backend
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.
By creating server/determine-status.js:
✅ Clean import: const { determineStatus } = require("../../server/determine-status")
✅ No server overhead: Just the function and its minimal dependencies
✅ Proper unit test: Tests only the function logic
✅ Reusable: Function can now be imported anywhere without pulling in router code
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.
What I was asking for was a straightforward, technical explanation of the decision in the context of this codebase. The previous response listed benefits, but it didn’t clearly explain what concrete issue exists with importing from the backend here, or why this change is necessary rather than just stylistic.
And that is not possible via the api-server?
What are you talking about?
And why do we need to move the function for this?
Where would you like to use it and why?