Skip to content

SAI-5884: hook lucene Unloader into Solr container lifecycle#294

Open
magibney wants to merge 42 commits intofs/branch_9_7from
michaelgibney/unloading-terms-and-dvs
Open

SAI-5884: hook lucene Unloader into Solr container lifecycle#294
magibney wants to merge 42 commits intofs/branch_9_7from
michaelgibney/unloading-terms-and-dvs

Conversation

@magibney
Copy link
Collaborator

@magibney magibney commented Oct 10, 2025

Note

Integrates Lucene Unloader across CoreContainer and Directory lifecycle with a dedicated executor, metrics, coordination points, and adds test/gradle options to control unloading.

  • Core/Container:
    • Add ScheduledExecutorService (unloaderExecutor) in CoreContainer with lifecycle management and accessor.
    • New org.apache.solr.core.UnloadHelper supplying Lucene Unloader.UnloadHelper, tracking metrics, and reacting to cluster props.
  • Directory/Storage:
    • CachingDirectoryFactory: wire UnloaderCoordinationPoint.setUnloadHelperSupplier(...); cache CoreContainer weak ref; initialize unload helper from object cache.
    • TeeDirectory: implement UnloaderCoordinationPoint; propagate unload helper to access and persistent directories.
    • TeeDirectoryFactory: tolerate null CoreContainer; keep weak ref volatile; initialize node-level state via ObjectCache.
  • Metrics:
    • Extend MetricUtils to support custom histogram/timer snapshot writing via SnapshotWriter.
  • Build/Tests:
    • Add Gradle test options: lucene.unload.ttl, lucene.unload.initial, lucene.unload.disable, lucene.unload.trackReloads.
    • CI: pass unload params in Solr test workflow.

Written by Cursor Bugbot for commit fc53df4. This will update automatically on new commits. Configure here.

cursor[bot]

This comment was marked as outdated.

@magibney magibney force-pushed the michaelgibney/unloading-terms-and-dvs branch from 20a9197 to 1f35b45 Compare October 20, 2025 16:56
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@magibney magibney force-pushed the michaelgibney/unloading-terms-and-dvs branch from a6953e1 to 6bd3222 Compare November 3, 2025 18:48
@magibney magibney force-pushed the michaelgibney/unloading-terms-and-dvs branch from d547e3b to ab94a74 Compare November 10, 2025 19:01
@magibney magibney force-pushed the michaelgibney/unloading-terms-and-dvs branch from 858d109 to 23a4b50 Compare November 21, 2025 19:27
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

This is the final PR Bugbot will review for you during this billing cycle

Your free Bugbot reviews will reset on December 18

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

if (dynamic < 0) {
threshold = minSegSizeForUnload;
} else {
threshold = dynamicMinSegSize;
Copy link

Choose a reason for hiding this comment

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

Bug: Volatile field read twice causing potential race condition

The code reads the volatile field dynamicMinSegSize into a local variable dynamic but then reads dynamicMinSegSize again instead of using the local variable. Since dynamicMinSegSize is volatile and can be modified by another thread, this creates a race condition where the threshold check could use a different value than was tested in the conditional. The assignment should be threshold = dynamic to use the cached value.

Fix in Cursor Fix in Web

} catch (Exception e) {
log.warn("problem parsing unloadMinSegSize spec=\"{}\"", o, e);
}
return true;
Copy link

Choose a reason for hiding this comment

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

Bug: Cluster properties listener incorrectly removed after first call

The second registerClusterPropertiesListener for unloadMinSegSize returns true at line 148, but according to the ClusterPropertiesListener interface contract, returning true means "the listener should be removed". This causes the listener to unregister itself after the first cluster properties change notification, preventing it from responding to future changes to unloadMinSegSize. The first listener for minUnloadTime correctly returns false to remain active.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants