Remove global block factory #100108
Merged
Remove global block factory #100108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@nik9000 EsqlDisruptionIT seems to fail more frequently on your machine compared to mine, possibly due to the different number of CPUs. I was able to reproduce reliably the problem on my machine using |
elasticsearchmachine
added
the
needs:triage
dnhatn
added
:Analytics/ES|QL
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
costin
approved these changes
Oct 1, 2023
|
@costin @ChrisHegarty Thanks for reviewing. |
piergm
pushed a commit
to piergm/elasticsearch
that referenced
this pull request
Oct 2, 2023
The global BlockFactory should work fine in production, where each Elasticsearch node runs in its own JVM process. However, this approach can lead to issues during testing, especially in IT tests. The same JVM process might get reused across multiple tests, resulting in situations where multiple IT tests inadvertently use the same instance of the global BlockFactory. For instance, EsqlDisruptionIT fails because it accidentally uses the global BlockFactory initialized by EsqlActionBreakerIT, which has a limit set to 1KB. Another issue in IT tests is that multiple Elasticsearch nodes can share the same (single) global instance of the BlockFactory. Closes elastic#100105
jakelandis
pushed a commit
to jakelandis/elasticsearch
that referenced
this pull request
Oct 2, 2023
The global BlockFactory should work fine in production, where each Elasticsearch node runs in its own JVM process. However, this approach can lead to issues during testing, especially in IT tests. The same JVM process might get reused across multiple tests, resulting in situations where multiple IT tests inadvertently use the same instance of the global BlockFactory. For instance, EsqlDisruptionIT fails because it accidentally uses the global BlockFactory initialized by EsqlActionBreakerIT, which has a limit set to 1KB. Another issue in IT tests is that multiple Elasticsearch nodes can share the same (single) global instance of the BlockFactory. Closes elastic#100105
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The global BlockFactory should work fine in production, where each Elasticsearch node runs in its own JVM process. However, this approach can lead to issues during testing, especially in IT tests. The same JVM process might get reused across multiple tests, resulting in situations where multiple IT tests inadvertently use the same instance of the global BlockFactory.
For instance, EsqlDisruptionIT fails because it accidentally uses the global BlockFactory initialized by EsqlActionBreakerIT, which has a limit set to 1KB. Another issue in IT tests is that multiple Elasticsearch nodes can share the same (single) global instance of the BlockFactory.
Closes #100105
This PR suggests removing the global BlockFactory since we are already passing it with the DriverContext.