refactor: add AndFilter and break down existing filters #64
Conversation
This file contains 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
brianluong
reviewed
Sep 19, 2022
| for i := range h.configs { | ||
| config := h.configs[i] | ||
| zap.L().Debug("Running healthchecks on upstream.", zap.String("upstreamID", config.ID)) | ||
| for range h.healthCheckTicker.C { |
internal/route/node_filter.go
Outdated
| return result | ||
| } | ||
|
|
||
| type IsHealthyFilter struct { |
There was a problem hiding this comment.
nit: do we need Filter in the name? The other structs don't have it.
There was a problem hiding this comment.
Or maybe just HealthyFilter? I feel like this Is prefix might be a little verbose. Indeed it's the general naming convention for boolean methods but I feel like the intent is already communicated clearly without the Is prefix.
There was a problem hiding this comment.
Went with IsHealthy for consistency with the others.
IvanVergiliev
force-pushed
the
composite-filters
branch
from
aadd18b
to
704f56f
Compare
IvanVergiliev
force-pushed
the
composite-filters
branch
from
704f56f
to
cef5348
Compare
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.
Description
This PR introduces an
AndFilterthat combines multipleNodeFilters and only passes if all sub-filters pass. This makes it very easy to add new filters and mix-and-match existing filters. The specific reason I'm adding this is to that I can add height-based or request-based filters in a follow-up PR and combine those with the existing ones.Some high-level notes:
HealthCheckManagerto use atime.Tickerfor testability. I wanted to confirm that theRunCheckcalls are executed as expected without waiting 5 seconds in the test, so I switched thetime.Sleepcall with an injected Ticker. That way, in the test, I can forceRunCheckinvocations by emitting an item on the Ticker channel.NodeFilterTypeenum and add afilters:section in the YAML config so people can choose which filters to use.node_filter.gois getting a bit long. The individual types and functions are pretty short still so it seems tolerable. We can break it up into multiple files or a package when it gets unwieldy.Type of change
How Has This Been Tested?
Unit tests for
AndFilterand updated test for theHealthCheckManager.