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

Apply the reader wrapper on can_match source #78988

Merged
merged 3 commits into from
Oct 26, 2021

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Oct 12, 2021

Today we don't wrap the original index reader with the reader wrapper
plugin if it is acquired from the can_match source. We've made this distinction
to ensure that the wrapper plugin doesn't perform any IO during the
can_match phase.
Now that the security layer loads the role query lazily, this change
removes the special path and applies the wrapper in all cases.
It also adds an assert to ensure that the loading of the role query
is never done on a transport thread.

Today we don't wrap the original index reader with the reader wrapper
plugin if it is acquired from the can_match source. We've made this distinction
to ensure that the wrapper plugin doesn't perform any IO during the
can_match phase.
Now that the security layer loads the role query lazily, this change
removes the special path and applies the wrapper in all cases.
It also adds an assert to ensure that the loading of the role query
is never done on a transport thread.
@jimczi jimczi added >enhancement :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v8.0.0 v7.16.0 labels Oct 12, 2021
@jimczi jimczi requested a review from ywelsch October 12, 2021 12:43
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Oct 12, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for taking so long to review this, I had forgotten about it.

@jimczi jimczi added v7.16.1 and removed v7.16.0 labels Oct 26, 2021
@jimczi jimczi merged commit ecc7695 into elastic:master Oct 26, 2021
@jimczi jimczi deleted the reader_wrapper branch October 26, 2021 10:22
jimczi added a commit to jimczi/elasticsearch that referenced this pull request Oct 26, 2021
Today we don't wrap the original index reader with the reader wrapper
plugin if it is acquired from the can_match source. We've made this distinction
to ensure that the wrapper plugin doesn't perform any IO during the
can_match phase.
Now that the security layer loads the role query lazily, this change
removes the special path and applies the wrapper in all cases.
It also adds an assert to ensure that the loading of the role query
is never done on a transport thread.
jimczi added a commit that referenced this pull request Oct 26, 2021
Today we don't wrap the original index reader with the reader wrapper
plugin if it is acquired from the can_match source. We've made this distinction
to ensure that the wrapper plugin doesn't perform any IO during the
can_match phase.
Now that the security layer loads the role query lazily, this change
removes the special path and applies the wrapper in all cases.
It also adds an assert to ensure that the loading of the role query
is never done on a transport thread.
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Oct 26, 2021
* upstream/master: (209 commits)
  Enforce license expiration (elastic#79671)
  TSDB: Automatically add timestamp mapper (elastic#79136)
  [DOCS]  `_id` is required for bulk API's `update` action (elastic#79774)
  EQL: Add optional fields and limit joining keys on non-null values only (elastic#79677)
  [DOCS] Document range enrich policy (elastic#79607)
  [DOCS] Fix typos in 8.0 security migration (elastic#79802)
  Allow listing older repositories (elastic#78244)
  [ML] track inference model feature usage per node (elastic#79752)
  Remove IncrementalClusterStateWriter & related code (elastic#79738)
  Reuse previous indices lookup when possible (elastic#79004)
  Reduce merging in PersistedClusterStateService (elastic#79793)
  SQL: Adjust JDBC docs to use milliseconds for timeouts (elastic#79628)
  Remove endpoint for freezing indices (elastic#78918)
  [ML] add timeout parameter for DELETE trained_models API (elastic#79739)
  [ML] wait for .ml-state-write alias to be readable (elastic#79731)
  [Docs] Update edgengram-tokenizer.asciidoc (elastic#79577)
  [Test][Transform] fix UpdateTransformActionRequestTests failure (elastic#79787)
  Limit CS Update Task Description Size (elastic#79443)
  Apply the reader wrapper on can_match source (elastic#78988)
  [DOCS] Adds new transform limitation item and a note to the tutorial (elastic#79479)
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/index/IndexMode.java
#	server/src/test/java/org/elasticsearch/index/TimeSeriesModeTests.java
lockewritesdocs pushed a commit to lockewritesdocs/elasticsearch that referenced this pull request Oct 28, 2021
Today we don't wrap the original index reader with the reader wrapper
plugin if it is acquired from the can_match source. We've made this distinction
to ensure that the wrapper plugin doesn't perform any IO during the
can_match phase.
Now that the security layer loads the role query lazily, this change
removes the special path and applies the wrapper in all cases.
It also adds an assert to ensure that the loading of the role query
is never done on a transport thread.
ywelsch added a commit that referenced this pull request Jan 13, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with #51708) made searches fail.

This is a bug that was indirectly introduced by #78988.

Closes #82044
ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request Jan 13, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with elastic#51708) made searches fail.

This is a bug that was indirectly introduced by elastic#78988.

Closes elastic#82044
ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request Jan 13, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with elastic#51708) made searches fail.

This is a bug that was indirectly introduced by elastic#78988.

Closes elastic#82044
ywelsch added a commit that referenced this pull request Jan 14, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with #51708) made searches fail.

This is a bug that was indirectly introduced by #78988.

Closes #82044
ywelsch added a commit that referenced this pull request Jan 14, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with #51708) made searches fail.

This is a bug that was indirectly introduced by #78988.

Closes #82044

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >enhancement Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.16.0 v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants