Skip to content

force reselection on relay 500s#7529

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.

Sign up for GitHub

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

Merged
alecsavvy merged 6 commits into from
Feb 9, 2024
Merged

force reselection on relay 500s #7529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2f81cd1
force reselection on relay 500s
alecsavvy Feb 8, 2024
02abae2
add chain health to health status
alecsavvy Feb 9, 2024
9abb1b1
reorganize for ts
alecsavvy Feb 9, 2024
c766058
make relay request custom so it doesnt access data field
alecsavvy Feb 9, 2024
5768b5b
Revert last three commits
alecsavvy Feb 9, 2024
95c4f2e
add the throw
alecsavvy Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/libs/src/sdk/services/DiscoveryNodeSelector/DiscoveryNodeSelector.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -743,4 +743,9 @@ export class DiscoveryNodeSelector implements DiscoveryNodeSelectorService {
}
return undefined
}

async triggerReselection() {
// force reselection while avoiding current node
await this.select(this.selectedNode)
}
}
8 changes: 8 additions & 0 deletions packages/libs/src/services/web3Manager/Web3Manager.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -268,6 +268,14 @@ export class Web3Manager {
bail(e)
return
}

if (e.response.status >= 500) {

@raymondjacobsonraymondjacobsonFeb 8, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can avoid touching it here and instead made the change here
https://github.com/AudiusProject/audius-protocol/blob/2a6c888ef4bf4d2bd44a2b7e3229f6a409af98d6/packages/libs/src/sdk/services/DiscoveryNodeSelector/DiscoveryNodeSelector.ts#L217

@stereostevestereosteveFeb 9, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think Alec's point is that the above code doesn't actually use the mainline DN post wrapper linked here... but instead is using the identityService.relay function with a baseURL override.

Since there is a retry wrapper this will probably work, altho we might want to throw instead of return at line 276.

But yeah I think we could also change this around to have a more divergent code path for discoveryRelay that uses the DN post wrapper, and maybe even delete the identity relay code. Possibly in a later PR if we want a quick fix.

@raymondjacobsonraymondjacobsonFeb 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. game for this as a quick fix then

@alecsavvyalecsavvyFeb 9, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Steve is right and that's why I didn't put the change here initially and it's also why the post hook wasn't being called. I've pushed a change that does use the DN post wrapper though. This required chain_health being added to the health check as well.

@alecsavvyalecsavvyFeb 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I can revert this change and submit it in a later PR if we are good with the quick fix.

@raymondjacobsonraymondjacobsonFeb 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes!

// force reselection since we're using
// identity service to relay
await this.discoveryProvider?.discoveryNodeSelector?.triggerReselection()
throw e
}

// Otherwise, throw to retry
throw e
}
Expand Down