Skip to content

Do not request gaps to fill when the sub range is already completed#109212

Merged
tlrx merged 4 commits intoelastic:mainfrom
tlrx:2024/05/30/do-not-request-gaps-when-sub-range-is-completed
May 31, 2024
Merged

Do not request gaps to fill when the sub range is already completed#109212
tlrx merged 4 commits intoelastic:mainfrom
tlrx:2024/05/30/do-not-request-gaps-when-sub-range-is-completed

Conversation

@tlrx
Copy link
Copy Markdown
Member

@tlrx tlrx commented May 30, 2024

When the range to read (subRange) is already completed, we can optimize by returning early without asking for the range to write (range) to be fully filled and save one or more fetch requests.

@tlrx tlrx added >non-issue :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v8.15.0 labels May 30, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label May 30, 2024
Copy link
Copy Markdown
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

}

if (complete >= range.end()) {
if (subRange.end() <= complete) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we make this clear in the javadoc, i.e., if the subrange is already present we may not fill the range.

I think this may not mean a lot in practice in that we already do the tryRead which should catch this except for race conditions (and there is not that far between the two checks). I am good with it anyway, seems right to me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I pushed 583ff31 for the doc.

I think this may not mean a lot in practice in that we already do the tryRead which should catch this except for race conditions (and there is not that far between the two checks). I am good with it anyway, seems right to me.

Yes, it seemed more correct to me too.

Copy link
Copy Markdown
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

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

LGTM

@tlrx tlrx merged commit 3a5e940 into elastic:main May 31, 2024
@tlrx
Copy link
Copy Markdown
Member Author

tlrx commented May 31, 2024

Thanks all!

@tlrx tlrx deleted the 2024/05/30/do-not-request-gaps-when-sub-range-is-completed branch May 31, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >non-issue Team:Distributed Meta label for distributed team. v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants