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

rename ReloadPolicy onCommit to onCommitWithDelay #2235

Merged

Conversation

giovannicuccu
Copy link
Contributor

Here is the pull request for issue #1824 that we dicussed on discord.
I simply renamed the reload policy from OnCommit to OnCommitWithDelay and updated the reference in the documentation and tests.

I tried to implement an OnCommit policy for synchronous reload but I did not succeed.
Here are the main issues I faced:

  1. IndexReader and IndexWriter can be created indipendently (see test_index_on_commit_reload_policy_different_directories in core/test.rs ) and as far as I understand they share the access to the same directory. When an IndexWriter commits the changes are propagated to the file system, if an IndexReader is executed in a different process there is no simple way to notify the commit in a synchronous way because "there isn't a channel" between them (aside from the file system). I checked if there are some notifications for updating mmapped files but I did not find anything
  2. There semantic of the reload is "lost" as the callback is passed to the directory object that implements the commit notification in a different way (worker thread for mmap_directory, direct call inside atomic_write for ram_directory). If the ReloadPolicy is "directory implementation indipendent" there is the need to associate it to the callback.

If you think it'd feasible to implement a sync OnCommit policy I ask for some advice and I'll start to dig further into the problem.

@PSeitz
Copy link
Contributor

PSeitz commented Nov 3, 2023

Thanks! As of now you can implement sync behavior by manually reloading, which should be fine for most use cases

@PSeitz PSeitz merged commit ef603c8 into quickwit-oss:main Nov 3, 2023
3 checks passed
PSeitz pushed a commit that referenced this pull request Apr 10, 2024
* rename ReloadPolicy onCommit to onCommitWithDelay

* fix format issues

---------

Co-authored-by: Giovanni Cuccu <gcuccu@imolainformatica.it>
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