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

Streamline AsyncShardFetch#getNumberOfInFlightFetches #96545

Merged

Conversation

DaveCTurner
Copy link
Contributor

Avoids an O(#nodes) iteration by tracking the number of fetches directly.

Backport of #93632 to 7.17

Avoids an O(#nodes) iteration by tracking the number of fetches directly.

Backport of elastic#93632 to 7.17
@DaveCTurner DaveCTurner added >bug :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) backport v7.17.11 labels Jun 5, 2023
@DaveCTurner
Copy link
Contributor Author

This backports cleanly and is a good improvement in large clusters. We've seen no related trouble since it was introduced in 8.5 so I see no good reason not to backport it, but just checking with @henningandersen before I merge it.

@DaveCTurner
Copy link
Contributor Author

We've seen no related trouble since it was introduced in 8.5

Correction, sorry, this was only introduced in 8.8 (I was thinking of #96546). But IMO the assertions in this area are good and we haven't seen any related test failures since it was merged.

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

Notice that this landed in 8.8, not 8.5 AFAICS?

Feel free to omit my nit if you prefer a clean backport.

@@ -131,7 +128,7 @@ public synchronized FetchResult<T> fetchData(DiscoveryNodes nodes, Set<String> i
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ideally we'd also assert consistency between fetchingCount and cache here (or inside the brace one line up).

Suggested change
assert assertFetchingCountConsistent();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++ thanks, yes that'd make sense. I'll do that in a follow up (and apply it in main too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #96553

@DaveCTurner DaveCTurner merged commit 6a755e3 into elastic:7.17 Jun 5, 2023
@DaveCTurner DaveCTurner deleted the 2023-06-05-backport-93632-7.17 branch June 5, 2023 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport >bug :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v7.17.11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants