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

Test cache affinity #163

Merged
merged 5 commits into from
Sep 19, 2023
Merged

Test cache affinity #163

merged 5 commits into from
Sep 19, 2023

Conversation

aarshkshah1992
Copy link
Contributor

Supersedes #162.

@aarshkshah1992 aarshkshah1992 changed the base branch from main to aa/test-simulator September 19, 2023 10:39
Comment on lines 294 to 295
// no more than 5 cids from the cid list of 20 should get re-routed (25%)
assert.LessOrEqual(t, rerouteCount, 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be 5, rather than 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +288 to +290
if n.URL == nodes[0].URL {
rerouteCount++
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should 'reroute' be incremented when this doesn't match?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe reroute is meant to keep track of how often caboose returns a new joining node as the candidate node to fetch a cid in which case caboose is "rerouting" where it fetches that cid from. In that case, the increment here is good imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah what Amean said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're iterating over bad nodes here.

@AmeanAsad
Copy link
Contributor

Hey @aarshkshah1992 I think we should change notation of "bad" and "good" nodes in the affinity tests to maybe just "baseNodes" and "newNodes". I know I started with that notation in the test but I think its better to change it to make the test a bit more clear.

@aarshkshah1992
Copy link
Contributor Author

@AmeanAsad Done.

Copy link
Contributor

@AmeanAsad AmeanAsad left a comment

Choose a reason for hiding this comment

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

Looks good!

@aarshkshah1992 aarshkshah1992 merged commit 552ea1b into aa/test-simulator Sep 19, 2023
16 checks passed
@aarshkshah1992 aarshkshah1992 deleted the feat/cache-aff-test branch September 19, 2023 12:45
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