Add BrokerURL to the cache ad when broker is enabled #2792
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
- The broker is now used by both the Origin and Cache services. This commit moves its metrics to a common location so they can be accessed by both, rather than being coupled to the Origin.
- Checks Origin.EnableBroker or Cache.EnableBroker and sets the broker URL appropriately
- This mirrors the origin implementation and allows tracking of cache broker connections
h2zh
force-pushed
the
cache-broker-url
branch
from
c8a337e to
4e866ee
Compare
- The test verifies that: 1. The cache broker metric collector exists 2. The cache broker is properly enabled 3. The broker listener starts up successfully 4. The cache polls the broker endpoint (visible in logs) 5. Broker metrics can be incremented
h2zh
force-pushed
the
cache-broker-url
branch
from
4e866ee to
cc1f153
Compare
patrickbrophy
requested changes
Nov 13, 2025
|
|
||
| // SetBrokerURL sets the broker URL in the advertisement for servers that have broker support enabled. | ||
| // Returns the broker URL string if successful, or an empty string if not applicable. | ||
| func SetBrokerURL(ad *server_structs.OriginAdvertiseV2, serverType server_structs.ServerType, prefixes []string) error { |
Contributor
There was a problem hiding this comment.
Can you add tests for this function?
- Test with multiple prefixes: verify broker URL is not set
- Test with broker disabled: verify broker URL is not set
- Test with invalid broker endpoint: verify error is returned
| ) | ||
|
|
||
| // Spin up a federation and verifies that the cache broker infrastructure is properly configured. | ||
| func TestBrokerApi(t *testing.T) { |
Contributor
There was a problem hiding this comment.
This seems like a strange test. It appears that we are just manually incrementing the counter ourselves and then checking that it has increased? This doesn't really seem to test that the connection was actually made.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR consolidates the BrokerURL getter logics for Origin and Cache in their advertising process, and adds a new fed test for Broker-enabled Cache.
See the commit messages for details.
Not sure if this PR conflicts with the current Broker work you're doing @patrickbrophy