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

Reduce CacheImpl lock contention #5699

Closed
wants to merge 4 commits into from
Closed

Conversation

schlosna
Copy link
Contributor

@schlosna schlosna commented Jan 9, 2024

Description

When io.fabric8.kubernetes.client.informers.cache.Lister#list is used with a namespace under heavy concurrent load, the CacheImpl#byIndex becomes a performance bottleneck due to synchronized method contention.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

Copy link

sonarcloud bot commented Jan 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

78.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@manusa
Copy link
Member

manusa commented Jan 10, 2024

BasicItemStoreTest.java needs a header with the exact same content as header.txt (notice that the year is not the current year but the inception year -2015-)

@shawkins
Copy link
Contributor

shawkins commented Feb 8, 2024

@schlosna thank you for the pr. Just a couple of thoughts:

  • I don't think users should notice / care there's a small difference in consistency here between the cache and index state vs. the fully synchronized case. However a javadoc on CacheImpl could mention a serial usage expectation as parallel mutative operations could result in things like orphaned index entries - we currently enforce serialized execution of watch events via a SerialExecutor.
  • it would probably be better to have CacheImpl hold a special purpose lock object, or we'll need another release not letting implementors of ItemStores know that their object will be the lock used by the CacheImpl.
  • there's a few more places where locking may be good - removeIndexer goes against the pattern of locking on mutative index operations, and addIndexers produces an unsychronized copy of indexers.
  • it would be great to avoid explicit double locking under addIndexFunc, but it doesn't at first glance seem possible. Also I don't think there would be chance for deadlock - especially if we change the object lock to something held only by CacheImpl.

@manusa manusa added the Waiting on feedback Issues that require feedback from User/Other community members label Feb 26, 2024
This was referenced May 1, 2024
@shawkins shawkins closed this May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting on feedback Issues that require feedback from User/Other community members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants