Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Sync in-memory store w/ Consul at constant interval #278

Merged
merged 9 commits into from
Jul 22, 2022
Merged

Conversation

nathancoleman
Copy link
Member

@nathancoleman nathancoleman commented Jul 11, 2022

This is longer-term followup based on #230 (comment)

Changes proposed in this PR:

Trigger a sync for our in-memory store at a constant interval in the background

How I've tested this PR:

Still working on how to test this in a programmatic fasion

How I expect reviewers to test this PR:

Checklist:

  • Tests added
  • CHANGELOG entry added

    Run make changelog-entry for guidance in authoring a changelog entry, and
    commit the resulting file, which should have a name matching your PR number.
    Entries should use imperative present tense (e.g. Add support for...)

@nathancoleman nathancoleman force-pushed the config-sync branch 2 times, most recently from 534dad6 to 39e6a2b Compare July 11, 2022 22:50
@nathancoleman nathancoleman force-pushed the config-sync branch 2 times, most recently from 51b014e to 94755a4 Compare July 12, 2022 19:17
@nathancoleman nathancoleman marked this pull request as ready for review July 12, 2022 20:48
This is necessary because the sync is guarded to only run if a listener on the gateway is marked as needing to sync. In this case where we sync at an interval, a listener will usually not be marked and so the sync would be skipped even if it's needed due to some drift in the consul config.
)

var (
ErrCannotBindListener = errors.New("cannot bind listener")
consulSyncInterval = 60 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

we may want to consider moving this into a variable for command-line override eventually -- there should only be a few API calls per-gateway, but I would imagine that for some with large numbers of gateways deployed (not the normal case), this could cause problems down the line if it's too aggressive. That said, 60s seems like it's fairly safe in like 99.9% of use-cases (i.e. non-overloaded Consul server or low number of gateways).

@nathancoleman nathancoleman merged commit 05b83e9 into main Jul 22, 2022
@nathancoleman nathancoleman deleted the config-sync branch July 22, 2022 18:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants