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

Foreach processor - fork recursive call #50514

Merged
merged 9 commits into from Jan 8, 2020

Conversation

jakelandis
Copy link
Contributor

@jakelandis jakelandis commented Dec 27, 2019

A very large number of recursive calls can cause a stack overflow
exception. This commit forks the recursive calls for non-async
processors.

@drsantos20
Copy link

hi @jakelandis this pr is ready to review?

@jakelandis
Copy link
Contributor Author

@drsantos20 - not yet, while functional (as-is) it is introducing a bit more blocking then I would like. I am still exploring the options.

A very large number of recursive calls can cause a stack overflow
exception. This commit forks the recursive calls for non-async
processors.
@jakelandis jakelandis changed the title use iterative with semaphore for the for each processor Foreach processor - fork recursive call Jan 2, 2020
@jakelandis jakelandis marked this pull request as ready for review January 2, 2020 16:20
@jakelandis jakelandis added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v7.5.2 v8.0.0 labels Jan 2, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Ingest)

@jakelandis
Copy link
Contributor Author

jakelandis commented Jan 2, 2020

test failure due to: #50497

@jakelandis
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Left two minor comments, otherwise LGTM.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM2

@jakelandis
Copy link
Contributor Author

@elasticmachine test this please

@jakelandis
Copy link
Contributor Author

@elasticmachine run all the tests

@jakelandis
Copy link
Contributor Author

closing and re-opening in an attempt to get the tests to run.

@jakelandis jakelandis closed this Jan 8, 2020
@jakelandis jakelandis reopened this Jan 8, 2020
@jakelandis jakelandis merged commit c3de284 into elastic:master Jan 8, 2020
jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Jan 8, 2020
A very large number of recursive calls can cause a stack overflow
exception. This commit forks the recursive calls for non-async
processors. Once forked, each thread will handle at most 10
recursive calls to help keep the stack size and thread count
down to a reasonable size.
jakelandis added a commit that referenced this pull request Jan 9, 2020
A very large number of recursive calls can cause a stack overflow
exception. This commit forks the recursive calls for non-async
processors. Once forked, each thread will handle at most 10
recursive calls to help keep the stack size and thread count
down to a reasonable size.
jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Jan 9, 2020
A very large number of recursive calls can cause a stack overflow
exception. This commit forks the recursive calls for non-async
processors. Once forked, each thread will handle at most 10
recursive calls to help keep the stack size and thread count
down to a reasonable size.
jakelandis added a commit that referenced this pull request Jan 9, 2020
A very large number of recursive calls can cause a stack overflow
exception. This commit forks the recursive calls for non-async
processors. Once forked, each thread will handle at most 10
recursive calls to help keep the stack size and thread count
down to a reasonable size.
probakowski added a commit to probakowski/elasticsearch that referenced this pull request Jan 16, 2020
This change makes ForEachProcessor iterative and still non-blocking.
In case of non-async processors we use single for loop and no recursion at all.
In case of async processors we continue work on either current thread or thread
started by downstream processor, whichever is slower (usually processor thread).
Everything is synchronised by single atomic variable.

Relates elastic#50514
probakowski added a commit that referenced this pull request Jan 21, 2020
* Refactor ForEachProcessor to use iteration instead of recursion

This change makes ForEachProcessor iterative and still non-blocking.
In case of non-async processors we use single for loop and no recursion at all.
In case of async processors we continue work on either current thread or thread
started by downstream processor, whichever is slower (usually processor thread).
Everything is synchronised by single atomic variable.

Relates #50514
probakowski added a commit that referenced this pull request Jan 22, 2020
…#51104) (#51322)

* Refactor ForEachProcessor to use iteration instead of recursion (#51104)

* Refactor ForEachProcessor to use iteration instead of recursion

This change makes ForEachProcessor iterative and still non-blocking.
In case of non-async processors we use single for loop and no recursion at all.
In case of async processors we continue work on either current thread or thread
started by downstream processor, whichever is slower (usually processor thread).
Everything is synchronised by single atomic variable.

Relates #50514

* Update IngestCommonPlugin.java
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
A very large number of recursive calls can cause a stack overflow
exception. This commit forks the recursive calls for non-async
processors. Once forked, each thread will handle at most 10
recursive calls to help keep the stack size and thread count 
down to a reasonable size.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement v7.5.2 v7.6.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants