Skip to content

Conversation

@tlrx
Copy link
Member

@tlrx tlrx commented Feb 25, 2020

This pull request fixes the backward/forward seeking counters in searchable snapshot statistics. These counters use the current file pointer position and the new seeking position to compute the seeking stats, but at the time the stat is computed the current file pointer position is already updated to the new position, making the delta always equals to 0.

Sadly, no tests were able to catch this.

@tlrx tlrx added >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Feb 25, 2020
@tlrx tlrx requested a review from DaveCTurner February 25, 2020 13:07
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Oops, that'd explain it 😁

Do we need to distinguish lastSeekPosition from lastReadPosition? I think they only differ between a seek and the subsequent read.

@tlrx
Copy link
Member Author

tlrx commented Feb 25, 2020

Do we need to distinguish lastSeekPosition from lastReadPosition?

This allows to track seeks independently from reads (like it is done in the new added tests) and track stats for access patterns like read -> seek to A -> seek to B -> read.

If we don't distinguish lastSeekPosition from lastReadPosition then seeking counters are tracking the seeking length from the last read position; access patterns like read -> seek to A -> seek to B -> read won't be correctly tracked in stats. I do agree that it is not the common access pattern for Lucene files, but at least we're tracking seeks correctly. If we're only interested in tracking seeks that are followed by a read operations then maybe we should compute this information as a read position delta in non-contiguous read stats.

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Ok, makes sense. It'll be interesting to see if we do ever seek twice between reads like that.

LGTM

@tlrx tlrx merged commit c049b02 into elastic:feature/searchable-snapshots Feb 25, 2020
@tlrx tlrx deleted the fix-seekings branch February 25, 2020 15:19
@tlrx
Copy link
Member Author

tlrx commented Feb 25, 2020

Thanks David!

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

Labels

>bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants