Skip to content

[CAE-1046] fix(loading): cache the loaded flag for slave nodes #3410

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

Merged
merged 3 commits into from
Jun 18, 2025

Conversation

ndyakov
Copy link
Member

@ndyakov ndyakov commented Jun 17, 2025

Checking the loading state by sending a ping on each node selections increases the latency. See comments in #3370 .
This introduces a naive cache that should improve the latency once the node is marked as loaded

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a caching mechanism to skip repeated loading checks for slave nodes once they’re determined to be “loaded.”

  • Introduces an atomic loaded flag to clusterNode
  • Resets loaded when a node is marked as failing
  • Updates Loading() to short-circuit and set loaded after the first non-loading result
Comments suppressed due to low confidence (1)

osscluster.go:343

  • [nitpick] The field name loaded is a bit ambiguous. Consider renaming it to something like isLoadComplete or loadCached to clarify that it’s a one-time cache flag.
loaded     uint32 // atomic

@ndyakov ndyakov force-pushed the ndyakov/CAE-1046-cache-loading-state branch from 867a693 to 49c07d1 Compare June 17, 2025 11:03
@ndyakov ndyakov marked this pull request as ready for review June 17, 2025 11:48
Copy link
Contributor

@LINKIWI LINKIWI left a comment

Choose a reason for hiding this comment

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

Thanks! I agree this should resolve the latency regression introduced in v9.9.0 for the typical case slave request.

Ideally (in the future) we can update the loading state as a side effect of user command execution, in order to obviate the need for an explicit additional critical-path PING.

@ndyakov ndyakov merged commit f4358ac into master Jun 18, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants