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

Test cache affinity #162

Closed
wants to merge 2 commits into from

Conversation

aarshkshah1992
Copy link
Contributor

No description provided.

Copy link
Contributor

@willscott willscott left a comment

Choose a reason for hiding this comment

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

You've changed this test to make the 'good' nodes better, and then confirm that they're still the same ones for a set of CIDs.

I think this no longer exercises the original point of the test - In the modified version there is no change in the active ring from what I can tell. The goal of this test, i think is to aim to demonstrate that even as some new nodes join the ring, there is relative stability of the ring space otherwise.

Comment on lines +258 to +261
aff := ch.Caboose.GetAffinity(ctx)
if aff == "" {
aff = fmt.Sprintf(blockPathPattern, c)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this test should only be doing one of these, so shouldn't need this complexity?

@aarshkshah1992
Copy link
Contributor Author

@willscott The goal is to demonstrate that we always get the same nodes for the same cid (as long as the hash ring does not change). Adding nodes that are as good as the original ones will change the hash ring and re reroute content.

What kind of test do you have in mind here ?

@AmeanAsad
Copy link
Contributor

Hey @aarshkshah1992 @willscott I think Aarsh's test here might still serve as a good base case test for cache affinity separate from the original intended test I wrote.

That way we have two test cases:

  • Testing that the hash ring only maps cids to good nodes.
  • Testing stability in the ring space as new good nodes join the pool .

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.

3 participants