Skip to content
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.

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

test: Use interface for passing instance ring to partition instance ring #597

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

benclive
Copy link
Contributor

@benclive benclive commented Oct 7, 2024

What this PR does:

Define a InstanceRingReader interface for creating a new PartitionInstanceRing.

  • This is to enable mocking the Partition Instance Ring during tests external to this package.
  • Specifically, Loki already uses an ingester ring mock to test certain functionality, such as looking up ingesters for querying, without defining a full, in-memory ring. I would like to extend this test to use a partition instance ring without rewriting the whole test suite, but this isn't possible right now.
  • I cannot use approaches such as &Ring{ringDesc: &Desc{}} outside of the ring package because ringDesc is private, and the existing ReadRing interface does not provide the GetInstance(string) method.

Which issue(s) this PR fixes:
n/a

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2024

CLA assistant check
All committers have signed the CLA.

@pstibrany
Copy link
Member

This change seems unnecessary to me. You can as well just keep reference to the ring provided to NewPartitionInstanceRing function, without introducing new interface.

@benclive
Copy link
Contributor Author

benclive commented Oct 7, 2024

keep reference to the ring provided to NewPartitionInstanceRing function

The problem I want to solve, which isn't present in this repo, is to be able to provide a mocked ring implementation to NewPartitionInstanceRing so I can test external behaviour that depends on the it without initialising a full Ring, KVStore & running service.

Specifically, in Loki I'm looking to provide a static list of ingesters & partitions in tests so I can assert that the query engine generates the correct behaviour in various scenarios. I can't easily do that right now because I need to provide a real Ring implementation to the constructor.

I'm aware it is possible to set up the real implementation for a test but it requires a lot of setup that isn't related to what I actually want to test, so adding an interface here seems simpler, quicker and less error prone approach to testing component behaviour.

@pstibrany
Copy link
Member

I see that my idea doesn't solve your problem. I've checked the Mimir code and we don't call partitionRing.InstanceRing() method, approved.

@benclive benclive merged commit 53283a0 into main Oct 7, 2024
5 checks passed
@benclive benclive deleted the add-interface-for-partition-instance-ring branch October 7, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants