Skip to content

Conversation

@dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 5, 2020

Using a Long alone is not strong enough for the id of search contexts because we reset the id generator whenever a data node is restarted. This can lead to two issues:

  • Fetch phase can fetch documents from another index
  • A scroll search can return documents from another index

This commit avoids these issues by adding a UUID to the search context Id.

@dnhatn dnhatn added >bug :Search/Search Search-related issues that do not fall into other categories :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. v8.0.0 v7.7.0 labels Mar 5, 2020
@dnhatn dnhatn requested review from jimczi and jpountz March 5, 2020 00:14
@elasticmachine
Copy link
Collaborator

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

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@dnhatn
Copy link
Member Author

dnhatn commented Mar 8, 2020

@jimczi I looked into how we can reopen readers on other nodes in case the current node crashes. We can use commit_id or seq_id for this. However, we won't be able to utilize it until we have reader contexts as search contexts are still stateful on the master branch. I add a TODO for this. I think it's ready for review. Can you please take a look? Thank you!

@dnhatn dnhatn requested review from jimczi and jpountz and removed request for jimczi and jpountz March 8, 2020 04:37
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

The change looks good to me.

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM2

@dnhatn
Copy link
Member Author

dnhatn commented Mar 9, 2020

@jpountz @jimczi Thank you for reviews.

@dnhatn dnhatn merged commit b2ea329 into elastic:master Mar 9, 2020
@dnhatn dnhatn deleted the search-context-uuid branch March 9, 2020 15:59
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Mar 11, 2020
Using a Long alone is not strong enough for the id of search contexts
because we reset the id generator whenever a data node is restarted.
This can lead to two issues:

1. Fetch phase can fetch documents from another index
2. A scroll search can return documents from another index

This commit avoids these issues by adding a UUID to SearchContexId.
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Mar 11, 2020
dnhatn added a commit that referenced this pull request Mar 11, 2020
Using a Long alone is not strong enough for the id of search contexts
because we reset the id generator whenever a data node is restarted.
This can lead to two issues:

1. Fetch phase can fetch documents from another index
2. A scroll search can return documents from another index

This commit avoids these issues by adding a UUID to SearchContexId.
dnhatn added a commit that referenced this pull request Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. :Search/Search Search-related issues that do not fall into other categories v7.7.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants