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

Port Delete By Query to Reindex infrastructure #18329

Merged
merged 1 commit into from May 19, 2016

Conversation

Projects
None yet
5 participants
@tlrx
Copy link
Member

commented May 13, 2016

This commit ports the Delete By Query implementation on the same infrastructure as the Reindex and Update By Query APIs.

The Delete By Query shares most of its code with the Update By Query feature. Some code has been moved around so that there's no duplication. Reindex does not use it for now but the changes will help that. There's surely some improvements to do but I'd be happy to get some feedback from a review.. @nik9000 Can you please have a look? That'd be awesome.

Closes #16883


experimental[The delete-by-query API is new and should still be considered experimental. The API may change in ways that are not backwards compatible]

The simplest usage of `_delete_by_query` just performs a deletion on every

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

I think maybe you shouldn't be allowed to _delete_by_query with specifying a query? _update_by_query is less dangerous I think....

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

I agree, this is a doc issue. The code checks for the presence of a query and do not allow _delete_by_query usage without a query.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 17, 2016

Contributor

Cool! I'd just replace the whole sentence with something like "_delete_by_query performs a delete on every document in an index that matches a query. For example:" Or something like that.

@nik9000

View changes

docs/reference/docs/delete-by-query.asciidoc Outdated
and when the delete request is processed. When the versions match the document
is deleted.

All delete and query failures cause the `_delete_by_query` to abort and are

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

I think this is a part of update-by-query and reindex and delete-by-query's docs that'll have to be changed. Rejected executions are retried for bulk, and, with #18331, will be for searches.

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

Agreed - I changed that... Let me know if it's better.

@nik9000

View changes

docs/reference/docs/delete-by-query.asciidoc Outdated
returned by the failing bulk request are returned in the `failures` element so
it's possible for there to be quite a few.

If you want to simply count version conflicts not cause the `_delete_by_query`

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

This sentence is a bit funky - Maybe "If you'd like to count version conflicts rather than cause them to abort the _delete_by_query then set..." ? Something like that?

@nik9000

View changes

docs/reference/docs/delete-by-query.asciidoc Outdated
// CONSOLE
// TEST[setup:twitter]

By default `_delete_by_query` uses scroll batches of 100. You can change the

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

1000 now, I think.

@nik9000

View changes

...ndex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java Outdated
return bulkRequest;
}

/**
* Copies the metadata from a hit to the index request.
* Used to accept or ignore a search hit. Ignored search hits will be excluded
* from the bulk request.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

Might want to mention that this is also where we fail on invalid search hits. I expect delete-by-query doesn't need to fail like this though.

@nik9000

View changes

...ndex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java Outdated
* Copy the routing from a search hit to the action request.
*/
protected void copyRouting(ActionRequest<?> request, String routing) {
if (request instanceof IndexRequest) {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

I'm not a fan of having to do all the instanceof checks but I'm not sure what the best way to avoid them is.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

Honestly maybe wrapping the requests in another object - there is one interface and two implementations and they handle either arm of the instanceof check?

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

I'm not a fan of having to do all the instanceof checks but I'm not sure what the best way to avoid them is.

Yes... This where my sentence in the description of the pull request "There's surely some improvements to do but I'd be happy to get some feedback" makes all its sense :)

Honestly maybe wrapping the requests in another object - there is one interface and two implementations and they handle either arm of the instanceof check?

There's a DocumentRequest class around, maybe I can use it...

This comment has been minimized.

Copy link
@nik9000

nik9000 May 17, 2016

Contributor

If it fits! Good luck!

@nik9000

View changes

...ndex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java Outdated
extends AbstractAsyncBulkByScrollAction<Request, BulkIndexByScrollResponse> {
public abstract class AbstractAsyncBulkIndexByScrollAction<Request extends AbstractBulkByScrollRequest<Request>>
extends AbstractAsyncBulkByScrollAction<Request>
implements BiFunction<ActionRequest<?>, SearchHit, ActionRequest<?>> {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

I might prefer passing a reference to the function rather rather than implementing it here.

/**
* Sets common options of {@link AbstractBulkByScrollRequest} requests.
*/
protected Request setCommonOptions(RestRequest restRequest, Request request) {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

Cool.

@nik9000

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/DeleteByQueryRequest.java Outdated
* <ul>
* <li>it's <tt>non-atomic</tt>, a delete-by-query may fail at any time while some documents matching the query have already been
* deleted</li>
* <li>it's <tt>try-once</tt>, a delete-by-query may fail at any time and will not retry it's execution. All retry logic is left to

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

This isn't really true. Things like conflicts=ignore and retry on bulk failure and, soon, retry on search failure, all count as retries to some degree.

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

I agree - this is a copy/paste from DBQ plugin.

@Override
public ActionRequestValidationException validate() {
ActionRequestValidationException e = super.validate();
if (getSearchRequest().indices() == null || getSearchRequest().indices().length == 0) {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

These fell like the same validations as are in update-by-query's request. Maybe a common superclass is in order?

@nik9000

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestDeleteByQueryAction.java Outdated
/*
* Passing the search request through DeleteByQueryRequest first allows
* it to set its own defaults which differ from SearchRequest's
* defaults. Then the parse can override them.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

s/parse/parseInternalRequest/

@nik9000

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportDeleteByQueryAction.java Outdated
}

/**
* Simple implementation of delete-by-query using scrolling and bulk.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

I'm not sure if the "simple" comment really applies to anything in reindex any more....

@nik9000

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportDeleteByQueryAction.java Outdated

@Override
protected boolean accept(SearchHit doc) {
return true;

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

May want to comment that you this explicitly disables check for _source.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

Maybe delete-by-query can disable requesting the source by default? The same we we enable requesting the version?

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

Good catch!

@nik9000

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportDeleteByQueryAction.java Outdated

@Override
protected ActionRequest<?> copyMetadata(ActionRequest<?> request, SearchHit doc) {
copyParent(request, fieldValue(doc, ParentFieldMapper.NAME));

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

Maybe also good to comment on how the superclass is much more update oriented and you override to skip all of that.

@nik9000

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportReindexAction.java Outdated
@@ -57,8 +59,9 @@

@Inject
public TransportReindexAction(Settings settings, ThreadPool threadPool, ActionFilters actionFilters,
IndexNameExpressionResolver indexNameExpressionResolver, ClusterService clusterService, ScriptService scriptService,
AutoCreateIndex autoCreateIndex, Client client, TransportService transportService) {
IndexNameExpressionResolver indexNameExpressionResolver, ClusterService clusterService,

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

The arguments don't change, right?

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

Right.. revert this one

return;
}
if (routingSpec.startsWith("=")) {
index.routing(mainRequest.getDestination().routing().substring(1));
super.copyRouting(request, mainRequest.getDestination().routing().substring(1));

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

Nice.

@nik9000

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportReindexAction.java Outdated
break;
default:
throw new IllegalArgumentException("Unsupported routing command");
case "keep":

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

Awe, but I like my properly formatted switch statements!

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

I reverted this one too. It looks like we're not consistent within the code base regarding switch statements

This comment has been minimized.

Copy link
@nik9000

nik9000 May 17, 2016

Contributor

Nope! There are heathens who like it with indented case statements and they are in the majority too! But we don't have rules for it.

@nik9000

View changes

modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportUpdateByQueryAction.java Outdated
protected IndexRequest buildIndexRequest(SearchHit doc) {
public ActionRequest<?> apply(ActionRequest<?> request, SearchHit doc) {
ActionRequest<?> result = super.apply(request, doc);
if (scriptApplier == null) {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

This feels like something the superclass should handle. Reindex and update-by-query can pass the applier into the async action and delete-by-query can pass null? Then the superclass doesn't have to implement BiFunction?

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

I agree... I changed that, let me know what you think.

@Override
protected void scriptChangedTTL(IndexRequest index, Object to) {
throw new IllegalArgumentException("Modifying [" + TTLFieldMapper.NAME + "] not allowed");
class UpdateByQueryScriptApplier extends ScriptApplier {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

I like the idea of the script applier!

* places - that is the responsibility of {@link AsyncBulkByScrollActionTests} which have more precise control to simulate failures but do
* not exercise important portion of the stack like transport and task management.
*/
public class DeleteByQueryCancelTests extends ReindexTestCase {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

At some point soon I'd like to merge all of these cancel tests into a single test. I hadn't done that at first because ReindexTestCase couldn't build an UpdateByQueryRequestBuilder but that isn't the case any more.

@nik9000

View changes

modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryCancelTests.java Outdated

@Override
protected int numberOfShards() {
// Only 1 shard

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

might want to explain why you can only have one shard.

private final CountDown blockAfter = new CountDown(MAX_DELETIONS);

@Override
public Engine.Delete preDelete(Engine.Delete delete) {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

I like this a lot more than I like the way I do update-by-query and reindex cancellation. Do you think as a followup you can port those to this test?

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

Do you think as a follow up you can port those to this test?

I think so. I can give it a try once this PR is merged.

@nik9000

View changes

modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryConcurrentTests.java Outdated
index("test", "test", String.valueOf(i * 10 + j), "field", j);
}
}
refresh();

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

I'd probably build these with indexRandom because it'll usually bulk which is usually faster.

@nik9000

View changes

modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryConcurrentTests.java Outdated
assertHitCount(client().prepareSearch("test").setSize(0).get(), docs * threads.length);

final CountDownLatch start = new CountDownLatch(1);
final AtomicReference<Throwable> exceptionHolder = new AtomicReference<>();

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

So it turns out that RandomizedRunner sets an uncaught exception handler which should make this not required. I think.

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2016

Author Member

I didn't know about it, thanks!

This comment has been minimized.

Copy link
@nik9000

nik9000 May 17, 2016

Contributor

I didn't know it until a few days ago!

@nik9000

View changes

modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryConcurrentTests.java Outdated
final Thread[] threads = new Thread[scaledRandomIntBetween(2, 5)];
final long docs = randomIntBetween(1, 50);
for (int i = 0; i < docs; i++) {
for (int j = 0; j < threads.length; j++) {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 16, 2016

Contributor

Maybe you should make j t instead.

@nik9000

View changes

modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryConcurrentTests.java Outdated
final Thread[] threads = new Thread[scaledRandomIntBetween(2, 5)];
final long docs = randomIntBetween(1, 50);
int t;

This comment has been minimized.

Copy link
@nik9000

nik9000 May 17, 2016

Contributor

I'd probably still declare t in the loop just because it is the normal thing to do.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

I reviewed again and it looks pretty good! I think maybe rebase to resolve the conflicts I just created by merging #18331 and I'll have another look in my morning with fresh eyes?

@tlrx tlrx force-pushed the tlrx:delete-by-query-on-reindex-infra branch May 18, 2016

@tlrx

This comment has been minimized.

Copy link
Member Author

commented May 18, 2016

@nik9000 Thanks again! I rebased and updated the code according to your comments.

@clintongormley clintongormley changed the title Port Delete By Query on Reindex infrastructure Port Delete By Query to Reindex infrastructure May 18, 2016

@nik9000

View changes

docs/reference/docs/delete-by-query.asciidoc Outdated

[source,js]
--------------------------------------------------
DELETE /twitter/_delete_by_query

This comment has been minimized.

Copy link
@nik9000

nik9000 May 18, 2016

Contributor

I kind of wonder if _delete_by_query should be a POST instead of a DELETE. You aren't really deleting the thing at the URL so maybe POST is more right because you are causing the _delete_by_query action to be taken? I dunno.

This comment has been minimized.

Copy link
@tlrx

tlrx May 19, 2016

Author Member

I think the first implementation was DELETE /_query. I personally like the DELETE /_delete_by_query but let's ask @clintongormley about this :)

This comment has been minimized.

Copy link
@clintongormley

clintongormley May 19, 2016

Member

I think we should move to POST, not least of all because some clients (eg JS) don't allow bodies with a DELETE request. Plus I agree with @nik9000's reasoning here

This comment has been minimized.

Copy link
@tlrx

tlrx May 19, 2016

Author Member

Thanks, let's do that then.

@nik9000

View changes

...ndex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java Outdated
* from copying search hit metadata (parent, routing, etc) to potentially
* transforming the {@link RequestWrapper} completely.
*/
protected RequestWrapper<?> apply(RequestWrapper<?> request, SearchHit doc) {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 18, 2016

Contributor

I think maybe this doesn't have to be protected any more? Maybe it doesn't even have to be a method any more and you can just call the scriptApplier directly?

This comment has been minimized.

Copy link
@tlrx

tlrx May 19, 2016

Author Member

Yes, let's remove this completly

@nik9000

View changes

...ndex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java Outdated
protected void copyRouting(IndexRequest index, SearchHit doc) {
index.routing(fieldValue(doc, RoutingFieldMapper.NAME));
protected void copyType(RequestWrapper<?> request, String type) {
request.setType(type);

This comment has been minimized.

Copy link
@nik9000

nik9000 May 18, 2016

Contributor

I wonder if these methods are important any more now that you have the RequestWrapper? Like maybe just call the RequestWrapper method directly?

This comment has been minimized.

Copy link
@tlrx

tlrx May 19, 2016

Author Member

Sure, I should have done that.

@nik9000

View changes

...ndex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java Outdated
* (if supported by this one)
*/
protected void copyTimestamp(RequestWrapper<?> request, Long timestamp) {
copyTimestamp(request, timestamp != null ? timestamp.toString() : null);

This comment has been minimized.

Copy link
@nik9000

nik9000 May 18, 2016

Contributor

This method still has some logic but maybe it should just be a final method on the RequestWrapper and it should delegate to an abstract method on RequestWrapper? That'd mean RequestWrapper has to be an abstract class but that is ok.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 18, 2016

Contributor

I think.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 18, 2016

Contributor

Or maybe none of that has to be true because _ttl only supported on the wrapper that wraps Index requests.

This comment has been minimized.

Copy link
@tlrx

tlrx May 19, 2016

Author Member

I removed the copy* methods and let the wrapper to handle this correctly.

@nik9000

View changes

...ndex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java Outdated
private final CompiledScript script;
protected final ScriptService scriptService;
protected final ClusterState clusterState;
private final ScriptApplier scriptApplier;

This comment has been minimized.

Copy link
@nik9000

nik9000 May 18, 2016

Contributor

If you made this a BiFunction then you could get away with the noop implementation being just (a, b) -> a and save a ton of code! buildScriptApplier would have to return a BiFunction as well, but extensions to this class could still build an extension of ScriptApplier just like they do now.

This comment has been minimized.

Copy link
@tlrx

tlrx May 19, 2016

Author Member

Yeah, I hesitated a lot about this. Happy to have your opinion! Let's make buildScriptApplierreturns the BiFunction and that will be more simpler.

@@ -39,6 +39,7 @@ public RestRethrottleAction(Settings settings, RestController controller, Client
super(settings, client);
this.action = action;
controller.registerHandler(POST, "/_update_by_query/{taskId}/_rethrottle", this);
controller.registerHandler(POST, "/_delete_by_query/{taskId}/_rethrottle", this);

This comment has been minimized.

Copy link
@nik9000

nik9000 May 18, 2016

Contributor

I think you also should add this to reindex.rethrottle.json in the rest spec.

This comment has been minimized.

Copy link
@tlrx

tlrx May 19, 2016

Author Member

Good catch

@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 18, 2016

I'm getting a test failure with this:

==> Test Info: seed=D21734D0466B8A8E; jvm=1; suite=1
Suite: org.elasticsearch.smoketest.SmokeTestReindexWithGroovyIT
  2> REPRODUCE WITH: gradle :qa:smoke-test-reindex-with-groovy:integTest -Dtests.seed=D21734D0466B8A8E -Dtests.class=org.elasticsearch.smoketest.SmokeTestReindexWithGroovyIT -Dtests.method="test {yaml=update_by_query/10_script/Setting bogus ctx is an error}" -Des.logger.level=WARN -Dtests.security.manager=true -Dtests.jvms=12 -Dtests.locale=es-PE -Dtests.timezone=Asia/Dushanbe
FAILURE 0.21s | SmokeTestReindexWithGroovyIT.test {yaml=update_by_query/10_script/Setting bogus ctx is an error} <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: the error message was expected to match the provided regex but didn't
   > Expected: Invalid fields added to ctx \[junk\]
   >      but: was "{root_cause=[{type=illegal_argument_exception, reason=Invalid fields added to context [junk]}], type=illegal_argument_exception, reason=Invalid fields added to context [junk]}"
   >    at __randomizedtesting.SeedInfo.seed([D21734D0466B8A8E:5A430B0AE897E776]:0)
   >    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
   >    at org.elasticsearch.test.rest.section.DoSection.execute(DoSection.java:117)
   >    at org.elasticsearch.test.rest.ESRestTestCase.test(ESRestTestCase.java:399)
   >    at java.lang.Thread.run(Thread.java:745)
  2> NOTE: leaving temporary files on disk at: /home/manybubbles/Workspaces/Elasticsearch/elasticsearch/qa/smoke-test-reindex-with-groovy/build/testrun/integTest/J0/temp/org.elasticsearch.smoketest.SmokeTestReindexWithGroovyIT_D21734D0466B8A8E-001
  2> NOTE: test params are: codec=Asserting(Lucene60): {}, docValues:{}, maxPointsInLeafNode=1342, maxMBSortInHeap=5.027602980016196, sim=ClassicSimilarity, locale=es-PE, timezone=Asia/Dushanbe
  2> NOTE: Linux 3.16.0-4-amd64 amd64/Oracle Corporation 1.8.0_72-internal (64-bit)/cpus=12,threads=1,free=444417848,total=514850816
  2> NOTE: All tests run in this JVM: [SmokeTestReindexWithGroovyIT]
Completed [1/1] in 8.49s, 21 tests, 1 failure <<< FAILURES!

When I want to test "just" the reindex module I run gradle modules:reindex:check qa:smoke-test-reindex-with-groovy:check docs:check. Because all of those modules have reindex based tests, sadly.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 18, 2016

I left a few more comment, including a failing test. I'm pretty sure we're close to the end on this. I expect those are the last changes I'm likely to have any cause to suggest. After this is merged we'll need to do a few more things to finish the "delete-by-query porting project". This is my guess of the list:

  • Remove the delete-by-query plugin. Should we just remove it in 5.0.0, call it a breaking change, and tell people to use the version in the reindex module? Is that ok because we're kind of just moving the API? It is losing some small functionality (timeouts) but it is gaining cancel, throttle, and rethrottle.
  • Document all the breaking changes.
  • Consolidate all those tests that have a version of reindex and a version for update by query into single tests.
    ** Add a delete-by-query case to the new retry tests. That should probably wait until #18456 is merged.
@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 18, 2016

Oh! More things:

  • (optional) Implement ctx.op = "delete" on _update_by_query and _reindex.
  • Cut all the script based cancel tests over to your listener based one. That is nicer.
@tlrx

This comment has been minimized.

Copy link
Member Author

commented May 19, 2016

@nik9000 Thanks again! I updated the code and also fixed the failing test.

Once this one is merged, I'll address the remaining points one after one :)

@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 19, 2016

Once this one is merged, I'll address the remaining points one after one :)

I'm happy to grab a few!

@nik9000

View changes

docs/reference/docs/delete-by-query.asciidoc Outdated
@@ -31,15 +31,18 @@ That will return something like this:
{
"took" : 147,
"timed_out": false,
"deleted": 120,
"batches": 2,
"deleted": 119,

This comment has been minimized.

Copy link
@nik9000

nik9000 May 19, 2016

Contributor

119 seems weird. Maybe we're missing a refresh?

{
"took" : 147,
"timed_out": false,
"deleted": 119,

This comment has been minimized.

Copy link
@nik9000

nik9000 May 19, 2016

Contributor

119 feels wrong, like we're missing a refresh when we build the test data or something.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 19, 2016

Contributor

Oh! I see why, because of that one other tweet. Got it.

This comment has been minimized.

Copy link
@tlrx

tlrx May 19, 2016

Author Member

Yes :)

@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 19, 2016

LGTM

@tlrx tlrx force-pushed the tlrx:delete-by-query-on-reindex-infra branch to a01ecb2 May 19, 2016

@tlrx tlrx merged commit a01ecb2 into elastic:master May 19, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@tlrx tlrx removed the review label May 19, 2016

@tlrx tlrx deleted the tlrx:delete-by-query-on-reindex-infra branch May 20, 2016

@natelapp

This comment has been minimized.

Copy link

commented May 28, 2016

Will this change also support deleting external versioned document that was broken by the move to the plugin? The details of that bug are found in #16654.

@tlrx

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2016

@natelapp In the current state, no. Your issue is a specific case and I created #18750 to handle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.