Add UploadNodes to control uploads more tightly #559
Merged
Conversation
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
rickyrombo
commented
Nov 11, 2025
| }, | ||
| } | ||
|
|
||
| ProdUploadNodes = []string{ |
Contributor
Author
There was a problem hiding this comment.
need input on what these should be
Member
There was a problem hiding this comment.
i think this is good for now
raymondjacobson
approved these changes
Nov 11, 2025
Member
raymondjacobson
left a comment
raymondjacobson
left a comment
There was a problem hiding this comment.
have we tested this w/ local client, etc.?
| }, | ||
| } | ||
|
|
||
| ProdUploadNodes = []string{ |
Member
There was a problem hiding this comment.
i think this is good for now
schottra
approved these changes
Nov 11, 2025
| contentNodes := make([]contentNode, 0, len(healthyNodes)) | ||
| for _, node := range healthyNodes { | ||
| // icky reaching into config to check upload nodes | ||
| if slices.Contains(app.contentNodeMonitor.config.UploadNodes, node.Endpoint) { |
Contributor
There was a problem hiding this comment.
Can't we just read Config directly here? Not sure why we need to grab it from the monitor.
Contributor
Author
There was a problem hiding this comment.
wdym? Like config.Cfg? I would like to avoid globals but maybe you're right
Contributor
There was a problem hiding this comment.
Yeah I meant config.Cfg. Harder to test for sure.
Alternatively, I have often wondered why we don't make the config available on the app instance itself....
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Creates a configurable list of nodes that will be used for uploads (as they are returned by health_check)
Keeps existing health monitoring for all nodes, just filters at the end. A "smarter" method might be filtering before tracking health, but this gets the job done for now. Opted to do the filtering outside of the monitor as it might be confusing why it only has a small number of nodes it thinks are "healthy".
Notably still might be a footgun, as it's not obvious that health check would be limited to the UploadNodes