Skip to content

Use SequentialStoredFieldsLeafReader in reading Lucene changes#67190

Merged
dnhatn merged 1 commit intoelastic:masterfrom
dnhatn:sequential-changes-reader
Jan 12, 2021
Merged

Use SequentialStoredFieldsLeafReader in reading Lucene changes#67190
dnhatn merged 1 commit intoelastic:masterfrom
dnhatn:sequential-changes-reader

Conversation

@dnhatn
Copy link
Copy Markdown
Member

@dnhatn dnhatn commented Jan 7, 2021

REVERTED:

Reading operations in Lucene changes is likely sequential and more efficient with SequentialStoredFieldsLeafReader.

Relates #62509

@dnhatn dnhatn added >enhancement :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. v8.0.0 v7.12.0 labels Jan 7, 2021
@dnhatn dnhatn requested review from jimczi and romseygeek January 7, 2021 20:50
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team. label Jan 7, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM. It would be nice to make this API a bit easier to use, but I'm going to open a separate PR to do that. Thanks @dnhatn

@dnhatn
Copy link
Copy Markdown
Member Author

dnhatn commented Jan 12, 2021

Thanks Alan.

@dnhatn dnhatn merged commit 6cbaaed into elastic:master Jan 12, 2021
@dnhatn dnhatn deleted the sequential-changes-reader branch January 12, 2021 13:43
dnhatn added a commit that referenced this pull request Jan 12, 2021
dnhatn added a commit that referenced this pull request Jan 12, 2021
Reading operations in Lucene changes is likely sequential and more
efficient with SequentialStoredFieldsLeafReader.

Relates #66944
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jan 12, 2021
…67190)

Reading operations in Lucene changes is likely sequential and more
efficient with SequentialStoredFieldsLeafReader.

Relates elastic#66944
dnhatn added a commit that referenced this pull request Jan 12, 2021
Reading operations in Lucene changes is likely sequential and more
efficient with SequentialStoredFieldsLeafReader.

Relates #66944
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Feb 5, 2021
dnhatn added a commit that referenced this pull request Feb 5, 2021
…67190)" (#68586)

This reverts commit 5fe0d67.

I benchmarked an improvement in CCR and found that changes introduced in 
#67190 made CCR 10 times slower (1816 seconds to 26515 seconds).

"total_read_remote_exec_time_millis" : 2651518
"total_read_remote_exec_time_millis" : 1816094

With concurrent indexing and Lucene merges, documents in segments are no 
longer sorted by sequence numbers. And if the index sorting is
specified, documents are never sorted by sequence numbers. Using a
mergeInstance stored field reader will decompress the whole block, which
we will not use.

Reverts #67190
dnhatn added a commit that referenced this pull request Feb 5, 2021
…68586)

Revert "Use SequentialStoredFieldsLeafReader to read Lucene changes (#67190)" (#68586)

This reverts commit 5fe0d67.

I benchmarked an improvement in CCR and found that changes introduced in
#67190 made CCR 10 times slower (1816 seconds to 26515 seconds).

"total_read_remote_exec_time_millis" : 2651518
"total_read_remote_exec_time_millis" : 1816094

With concurrent indexing and Lucene merges, documents in segments are no
longer sorted by sequence numbers. And if the index sorting is
specified, documents are never sorted by sequence numbers. Using a
mergeInstance stored field reader will decompress the whole block, which
we will not use.

Reverts #67190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Engine Anything around managing Lucene and the Translog in an open shard. >non-issue Team:Distributed Meta label for distributed team. v7.12.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants