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

Fix incorrect pagination in journaling #848

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

konalegi
Copy link

@konalegi konalegi commented Jun 9, 2022

This PR fixes two bugs:

  • Removes the retries principle, just because Chewy::Stash::Journal.entries methods as any es query have a limit of default 10 and in case you have more than 100 they are going to be ignored. Instead, pagination is based on the total_count will scan through all entires in the journal.
  • Another bug is a result of the assumption that entries in the query are sorted by created_at, but in reality, it's not.
    As an example imagine the following entries in the journal
1. Entry (id: 1, created_at: '12:01:00')
2. Entry (id: 3, created_at: '12:04:00')
3. Entry (id: 2, created_at: '12:03:00')
4. Entry (id: 4, created_at: '12:05:00')

and the current algorithm step by step

  1. fetch 2 items older than 12:00:00, as a result, entries with ids 1, 3 will be fetched (src)
  2. calculate the latest in the batch (src), latest created_at will be 12:04:00
  3. fetch 2 items older than12:04:00, as a result, we are going have entry with id: 4 only
  4. Entry with 2 will be lost

This bug fix is simple, fetch all entries ordered by created_at


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

@konalegi konalegi force-pushed the bugfix-fix-journal-pagination branch 2 times, most recently from 64492d8 to c3d94a1 Compare June 9, 2022 15:51
@konalegi konalegi changed the title Fix incorrectly paginated journaling Fix incorrectly paginated in journaling Jun 9, 2022
@konalegi konalegi marked this pull request as ready for review June 9, 2022 17:50
@konalegi konalegi changed the title Fix incorrectly paginated in journaling Fix incorrectly pagination in journaling Jun 10, 2022
@konalegi konalegi changed the title Fix incorrectly pagination in journaling Fix incorrect pagination in journaling Jun 10, 2022
lib/chewy/journal.rb Show resolved Hide resolved
@konalegi konalegi force-pushed the bugfix-fix-journal-pagination branch from c3d94a1 to a317c7b Compare June 10, 2022 15:48
@konalegi konalegi merged commit b82016f into master Jun 10, 2022
@konalegi konalegi deleted the bugfix-fix-journal-pagination branch June 10, 2022 17:16
cyucelen pushed a commit to cyucelen/chewy that referenced this pull request Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants