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

Rewritten list query, test and fix [#82] #83

Merged
merged 4 commits into from Feb 21, 2024

Conversation

miro-balaz
Copy link
Contributor

Solves #82

Adds test to expose slowness of list query (#82)
Original list query had maybe quadratic complexity (plot) in terms of number of rows with same name.
New query passes the test, it still has superlinear complexity. But it has to be taken with grain of salt as I meassured individual updates.

Test performs two subsequents batches of 1000 updates.

Orignal query

  • duration of first batch: 18 320 405 224ns
  • duration of subsequent batch: 105 316 382 474ns

New Query

  • duration of first batch: 2 372 000 975ns
  • duration of subsequent batch duration: 2 684 759 515ns

Future work: By having specialized query for getting single name,(no prefix or range), we can have even faster, constant time complexity of list.

@ktsakalozos
Copy link
Member

Thank you for this PR @miro-balaz stay tuned for a proper review.

@ktsakalozos
Copy link
Member

You can install MicroK8s with this patch with:

sudo snap install microk8s --classic --channel=latest/edge/dqlite-list

Copy link
Member

@berkayoz berkayoz left a comment

Choose a reason for hiding this comment

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

Hey @miro-balaz,

Proposed list query seems to perform better under general circumstances where number of updated rows(with same name) are generally small. So LGTM.

Before merging, could you please verify you have submitted the CLA form? (If you are a first time contributor) And could you also rebase the PR to include the CLA check workflow?

Thanks.

@ktsakalozos
Copy link
Member

@miro-balaz thank you for this work we would like to merge it. Could you please rebase and sign the CLA so we can distribute the code? Thank you.

@miro-balaz
Copy link
Contributor Author

miro-balaz commented Feb 20, 2024 via email

Copy link
Member

@berkayoz berkayoz left a comment

Choose a reason for hiding this comment

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

I've went ahead with the rebase, everything seems to be good to go.

I'll go ahead with the merge,
Thanks @miro-balaz

@miro-balaz
Copy link
Contributor Author

miro-balaz commented Feb 26, 2024 via email

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.

None yet

3 participants