Skip to content

Track search work by container; purge on delete#6872

Closed
labkey-jeckels wants to merge 9 commits intorelease25.7-SNAPSHOTfrom
25.7_fb_searchPurge
Closed

Track search work by container; purge on delete#6872
labkey-jeckels wants to merge 9 commits intorelease25.7-SNAPSHOTfrom
25.7_fb_searchPurge

Conversation

@labkey-jeckels
Copy link
Contributor

Rationale

Instead of waiting for the indexer to drain its queue before deleting a container, we can purge that work from the queue.

Changes

  • Track search work based on the container for the data
  • Purge work for the container being deleted

Tasks 📍

  • Manual Testing
  • Needs Automation
  • Verify Fix

public boolean drainQueue(PRIORITY priority, long timeout, TimeUnit unit) throws InterruptedException
public void purgeForContainer(Container container)
{
_runQueue.removeIf(i -> container.getEntityId().equals(i._containerId));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the task owning this item be notified? E.g. with TaskListener.indexError() or new method TaskListener.cancel(). Will the task ever know it is done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added in my most recent commit.

/** Lower priority indexing work. Intended for work placed in the queue as part of a call to DocumentProvider.enumerateDocuments() */
crawl,
/** Intended for content that needed indexing or reindexing because it was modified or created */
modified,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced just having Runnables < Item is sufficient? Runnables can create Runnables, and you really want those to be run at higher pri. I suppose that there is lower risk of this exploding the queue (compared to the Item explosion), but it still does sort of lead to doing a "breadth-first" expansion of the tasks instead of "depth-first".

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe that could be fixed by having "child" runnables be added to the front of the queue, effectively taking the place of the "parent" (expand in place).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert the change if it's too simplistic (I intentionally did that work as a separate commit). My overall takeaway from this work is that we were wildly inconsistent with prioritization and that other people (including myself) didn't understand how the priorities were supposed to be used.

The one place that I found where Runnables beget Runnables is ExperimentServiceImpl.indexMaterials() and its data counterpart. There, a bulk Runnable creates item Resources and another bulk Runnable. I think that's consistent with the flattened set of priorities that sort Resources first. Have I gotten myself confused?

@labkey-jeckels
Copy link
Contributor Author

Replaced by #6897

1 similar comment
@labkey-jeckels
Copy link
Contributor Author

Replaced by #6897

@labkey-jeckels labkey-jeckels deleted the 25.7_fb_searchPurge branch August 1, 2025 16:35
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