Core: remove delete-by-query

[mikemccand]

This method is exceptionally trappy. I think we should remove it and add it back only once we can do it safely.

It secretly does a refresh with each request, which means it makes changes visible if maybe you didn't want to (#3593).

It also means if you call this while concurrently index you can easily blow up the number of segments in the shard (merging can't keep up), leading to all sorts of problems e.g. OOME (##6025), super slow indexing, etc.

Finally, it can cause inconsistent docs indexed in primary vs. replica since the query is re-executed on the replica possibly matching different documents.

Until just recently we failed to throttle delete-by-query when merges were falling behind (#9986)

I think we should deprecate (remove in 2.0) for now, and only once we have task management should we add it back without all these traps (#7052).

[simonbrandhof]

If delete-by-query is removed, then how would you truncate an index ? Deleting and creating again the index requires to know metadata, which is not always possible.

[mikemccand]

If delete-by-query is removed, then how would you truncate an index

Deleting the entire index is much faster than delete-by-query of all docs in the index. But if that's not an option I suggest you run scan+scroll of your query to get all hits and then do bulk-request deletes of the returned ids.

[s1monw]

If delete-by-query is removed, then how would you truncate an index ? Deleting and creating again the index requires to know metadata, which is not always possible.

In this case I recommend to just read the metadata from the index and put it in your create request, then delete the index and you are done with it.

[s1monw]

@mikemccand I am not sure if we should do this in 1.5 but deprecating it I think is a no brainer?

[mikemccand]

@mikemccand I am not sure if we should do this in 1.5 but deprecating it I think is a no brainer?

Yeah my plan is deprecate in 1.5 and remove in 2.0.

[mikemccand]

I started to remove this in 2.0 but got stuck because we use delete-by-query internally to allow deleting an entire type from an index ... once we remove this in 2.0 (#8877) then we can do this issue.

[mikemccand]

Since #8877 is done I went and removed all delete-by-query logic but ... then I realized: what if a user has a DBQ in the translog on a shard and upgrades to 2.0? Must we support this back-compat case? Can we "require" that users flush (clears the translog) before upgrading?

If not ... I need to put back the DBQ logic for translog and Engine.

[s1monw]

If not ... I need to put back the DBQ logic for translog and Engine.

I am afraid we have to.

[rjernst]

But we can still remove the public API right? This can just be an internal backcompat implementation detail?

[mikemccand]

But we can still remove the public API right? This can just be an internal backcompat implementation detail?

Yeah ... I also need to fix the static back compat indices to leave some DBQs in the translog and confirm on upgrade the docs are in fact deleted ...

[mikemccand]

If not ... I need to put back the DBQ logic for translog and Engine.

I am afraid we have to.

Maybe we can simply detect when this ("DBQ in translog on upgrade") happens and refuse to start the shard, saying that you must go back to the prior version and flush the shard?

[rjernst]

That seems reasonable? This is an extreme edge case. If they are upgrading from 1.5+, the flush is done automatically on shutdown. And we already recommend running flush before any shutdown in general. So the only way for them to hit this is to have done a DBQ since the last time a flush happened (max 30 mins by default?), and then to have shutdown without doing a flush.

[mikemccand]

That seems reasonable? This is an extreme edge case. If they are upgrading from 1.5+, the flush is done automatically on shutdown.

Alas, the flush is only done in 1.5+ if a recovery was not also in process when the node was shut down, because a recovery blocks flushes (separately, we need to fix translog so it's ref counted instead: the two operations really should be independent).

Anyway, I think for this we should just keep the "replay DBQ on upgrade" ... the DBQ code that we need to keep around to do this is very small: just Engine, TransLog, IndexShard.

[kimchy]

++ on fixing current behavior of delete by query. I think we all agree on the fix, which is to do a scan search and bulk deletes, instead of using delete-by-query, since thats safer and more consistent with our replication model. It might be a long running task, but relatively lightweight, so I don't think we need to wait for the task management infra if we plan to remove delete-by-query in 2.0, and then add it back when we have it, but just go ahead and replace the current implementation with scan/delete and not deprecate it.

[bleskes]

+1 on giving the user an alternative, albeit a slowish instead of removing the current implementation first and then later adding a managed one.

[mikemccand]

I agree it would be great to "delete old way and add new way" in one go...

so I don't think we need to wait for the task management infra if we plan to remove delete-by-query in 2.0

That would be great, if it really is OK to just "be slow" sometimes (when the query deleted many docs). I guess there is precedent here: the optimize API (with wait_for_merge=true) can clearly take a very long time...

Somehow, whichever node receives the DBQ request, must open a client connection (to itself?), run the scan search, scroll through the hits and make bulk delete requests. I think #10251 is an example of how to do this ...

I'll add a comment on #7052 that we don't need to wait for task management API, and block this issue on it.

[mikemccand]

This issue is blocked on #7052.

[s1monw]

I don't think this need to be blocked on #7052 We can remove this now and add the other one as a followup. Stuff like this should not be blocked syntactic sugar

[mikemccand]

I don't think this need to be blocked on #7052

OK I opened #10288

[s1monw]

@mikemccand can we close this?

[mikemccand]

Yes.