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

Use TransportBulkAction for internal request from IndicesTTLService #5795

Merged
merged 1 commit into from Apr 15, 2014

Conversation

Projects
None yet
4 participants
@s1monw
Copy link
Contributor

commented Apr 14, 2014

This prevents executing bulks internal autocreate indices logic
and ensures that this internal request never creates an index
automaticall.

This fixes a bug where the TTL purger thread ran after the actual
index it was purging was already closed / deleted and that re-created
that index.

Closes #5766

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2014

@imotov I pushed a new commit based on your comments!

@imotov

View changes

src/test/java/org/elasticsearch/percolator/TTLPercolatorTests.java Outdated
@@ -148,6 +147,60 @@ public boolean apply(Object input) {
).execute().actionGet();
assertMatchCount(percolateResponse, 0l);
assertThat(percolateResponse.getMatches(), emptyArray());
ensureGreen();

This comment has been minimized.

Copy link
@imotov

imotov Apr 14, 2014

Member

Do we need this?

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

we don't I will fix

@imotov

This comment has been minimized.

Copy link
Member

commented Apr 14, 2014

Left a couple of comments with cosmetic changes. Everything else - LGTM.

@bleskes

This comment has been minimized.

Copy link
Member

commented Apr 15, 2014

I wonder if we should introduce another path for bulk request execution. Instead we can add an option to the bulk request to disable index creation per request? An alternative would be to use the BulkShardRequest directly as we are operating on a single shard and don't need all the fancy resolving logic of BulkRequest. Did you consider that?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2014

@bleskes this is a bigger change and I don't see any need to do this unless you want to refactor this class. This is a bugfix and should be least intrusive. I created a new issue #5809 to prevent this kind of behavior for delete only ops. We can optimize this later but I want to get this in first. Feel free to open a follup issue.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2014

I pushed new commits - I think it's ready

@imotov

View changes

src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java Outdated
}

private void executeBulk(final BulkRequest bulkRequest, final long startTime, final ActionListener<BulkResponse> listener) {
final AtomicArray<BulkItemResponse> responses = new AtomicArray<BulkItemResponse>(bulkRequest.requests.size());

This comment has been minimized.

Copy link
@imotov

imotov Apr 15, 2014

Member

I think we might miss some responses in case of onFailure() because it will be using responses created here

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

yeah you are right test fail all over the place :) I will revert this in a sec

Use TransportBulkAction for internal request from IndicesTTLService
This prevents executing bulks internal autocreate indices logic
and ensures that this internal request never creates an index
automaticall.

This fixes a bug where the TTL purger thread ran after the actual
index it was purging was already closed / deleted and that re-created
that index.

Closes #5766

@s1monw s1monw merged commit 8bede70 into elastic:master Apr 15, 2014

@s1monw s1monw deleted the s1monw:issues/5766 branch Apr 15, 2014

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.