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

Fixed filters execution order and fix potential concurrency issue in filter chains #7023

Merged
merged 2 commits into from Jul 28, 2014

Conversation

Projects
None yet
4 participants
@javanna
Copy link
Member

javanna commented Jul 24, 2014

Fix filters ordering which seems to be the opposite to what it should be (#7019). Added missing tests for rest filters.

Solved concurrency issue in both rest filter chain and transport action filter chain (#7021).

Closes #7019
Closes #7021

@javanna javanna added the review label Jul 24, 2014

@javanna javanna self-assigned this Jul 24, 2014

@javanna javanna changed the title Internal: fixed filters execution order and fix concurrency issue in filter chains Internal: fixed filters execution order and fix potential concurrency issue in filter chains Jul 25, 2014

@kimchy

View changes

src/main/java/org/elasticsearch/action/support/TransportAction.java Outdated
@@ -146,12 +148,12 @@ public void run() {

private class TransportActionFilterChain implements ActionFilterChain {

private volatile int index = 0;
private AtomicInteger index = new AtomicInteger();

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 25, 2014

Member

can this be final?

This comment has been minimized.

Copy link
@javanna

javanna Jul 28, 2014

Author Member

sure, will change that!

@kimchy

View changes

src/main/java/org/elasticsearch/rest/RestController.java Outdated
@@ -216,7 +215,7 @@ private String getPath(RestRequest request) {

private final RestFilter executionFilter;

private volatile int index;
private AtomicInteger index = new AtomicInteger();

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 25, 2014

Member

can this be final?

This comment has been minimized.

Copy link
@javanna

javanna Jul 28, 2014

Author Member

yep

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Jul 25, 2014

LGTM with 2 minor comments

javanna added some commits Jul 24, 2014

Internal: use AtomicInteger instead of volatile int for the current a…
…ction filter position

Also improved filter chain tests to not rely on execution time, and made filter chain tests look more similar to what happens in reality by removing multiple threads creation in testTooManyContinueProcessing (something we don't support anyway, makes little sense to test it).

Closes #7021

@javanna javanna merged commit 4e5ad56 into elastic:master Jul 28, 2014

@jpountz jpountz removed the review label Jul 29, 2014

@clintongormley clintongormley changed the title Internal: fixed filters execution order and fix potential concurrency issue in filter chains Internal: Fixed filters execution order and fix potential concurrency issue in filter chains Sep 10, 2014

@clintongormley clintongormley changed the title Internal: Fixed filters execution order and fix potential concurrency issue in filter chains Fixed filters execution order and fix potential concurrency issue in filter chains Jun 6, 2015

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.