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

enhance smoosh to cleanup search indexes when ddocs change #4718

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rnewson
Copy link
Member

@rnewson rnewson commented Aug 4, 2023

Overview

automate cleaning up search indexes (clouseau and nouveau) when design documents are updated.

Testing recommendations

TBD

Related Issues or Pull Requests

N/A

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • Documentation changes were made in the src/docs folder
  • Documentation changes were backported (separated PR) to affected branches

@@ -342,6 +345,8 @@ find_channel(#state{} = State, [Channel | Rest], Object) ->
find_channel(State, Rest, Object)
end.

stale_enough({?INDEX_CLEANUP, _}) ->
true;
Copy link
Member Author

Choose a reason for hiding this comment

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

ddoc_updated events are rare, unlike database updates, so choosing not to action one would leave unreferenced indexes on disk for a long time, perhaps indefinitely.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're also triggering these during every shard compaction attempt, proportionally to the number of shards (so we'd roughly trigger once for each clustered db). That should be a low enough rate, it may be worth checking what it looks like a busy cluster.

@rnewson
Copy link
Member Author

rnewson commented Aug 4, 2023

I need to add tests to this and intend to, just wanted to show the work so far.

@nickva
Copy link
Contributor

nickva commented Aug 7, 2023

Overall this looks like what we would want to happen: as soon as we update the ddoc we can clean up the old index data.

With the ddoc trigger one thing I was worried about is the case when we get a ddoc_updated on one node first, and not the others yet: we make a clustered call to get all the ddocs and get the signatures, what if that's stale since it happens concurrently with the ddoc_updated, would there be a chance we'd remove the new index file we just created? Maybe it's a low enough chance of happening that it's not a problem as long as it eventually sorts itself out. (We delete the newly building index file but then it gets rebuilt quickly anyway).

@rnewson
Copy link
Member Author

rnewson commented Aug 7, 2023

that's an excellent point, we must ensure that cannot happen, at least not with any higher probability than the existing cleanup code for mrview.

Comment on lines +618 to +623
cleanup_clouseau_indices(Dbs, ActiveSigs) ->
Fun = fun(Db) -> clouseau_rpc:cleanup(Db, ActiveSigs) end,
lists:foreach(Fun, Dbs).
cleanup_nouveau_indices(Dbs, ActiveSigs) ->
Fun = fun(Db) -> nouveau_api:delete_path(nouveau_util:index_name(Db), ActiveSigs) end,
lists:foreach(Fun, Dbs).
Copy link
Contributor

Choose a reason for hiding this comment

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

These shouldn't crash if either of those are not enabled? If there is a chance we could wrap them in a try ... catch maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

clouseau_rpc:cleanup is a gen_server:cast so that's fine even if the target node doesn't exist.

all the nouveau_api functions have a send_if_enabled check and return {error, nouveau_not_enabled} when it's not enabled.

in either case I ignore the function result, but perhaps I should check that it's one of the two expected results?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. The only worry was that we would throw an exception and prevent other indexes from getting cleaned up. Thanks for double-checking, I think it's fine as is, then.

@nickva
Copy link
Contributor

nickva commented Aug 17, 2023

that's an excellent point, we must ensure that cannot happen, at least not with any higher probability than the existing cleanup code for mrview.

We could punt changing the trigger mechanism for later and just opt to run the nouveau and dreyfus cleanup as is, alongside mrview indexes?

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