Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants