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.

Sign up for GitHub

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

Jump to bottom

Implement ping command for SQLite #2965

Merged
merged 16 commits into from
Jul 7, 2023
Merged

Implement ping command for SQLite #2965

merged 16 commits into from
Jul 7, 2023

Conversation

noisersup
Copy link
Member

@noisersup noisersup commented Jul 4, 2023 •
edited
Loading

Description

Closes #2756.

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@noisersup noisersup self-assigned this Jul 4, 2023
@noisersup noisersup added the not ready Issues that are not ready to be worked on; PRs that should skip CI label Jul 4, 2023
@noisersup noisersup removed the not ready Issues that are not ready to be worked on; PRs that should skip CI label Jul 4, 2023
@codecov
Copy link

codecov bot commented Jul 4, 2023 •
edited
Loading

Codecov Report

Merging #2965 (5af3cbf) into main (ea31cf5) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2965      +/-   ##
==========================================
- Coverage   76.07%   76.07%   -0.01%     
==========================================
  Files         385      385              
  Lines       20959    20977      +18     
==========================================
+ Hits        15945    15958      +13     
- Misses       4086     4088       +2     
- Partials      928      931       +3
Impacted Files Coverage Δ
internal/handlers/sqlite/msg_ping.go 66.66% <50.00%> (-33.34%) ⬇️

... and 6 files with indirect coverage changes

Flag Coverage Δ
hana ?
integration 72.56% <50.00%> (-0.01%) ⬇️
mongodb 5.36% <0.00%> (-0.01%) ⬇️
pg 65.99% <0.00%> (-0.06%) ⬇️
shard-1 55.07% <0.00%> (-1.24%) ⬇️
shard-2 56.43% <50.00%> (+0.07%) ⬆️
shard-3 55.54% <0.00%> (+0.78%) ⬆️
sqlite 45.93% <50.00%> (+0.02%) ⬆️
unit 24.41% <0.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@noisersup noisersup changed the title Implement ping command for SQLite Implement ping command for SQLite Jul 4, 2023
@noisersup noisersup added the code/chore Code maintenance improvements label Jul 4, 2023
@noisersup noisersup marked this pull request as ready for review July 4, 2023 13:40
@noisersup noisersup requested review from AlekSi and a team as code owners July 4, 2023 13:40
@noisersup noisersup requested a review from rumyantseva July 4, 2023 13:40
@noisersup noisersup enabled auto-merge (squash) July 4, 2023 13:43
@noisersup noisersup requested review from a team and chilagrow July 4, 2023 13:43
rumyantseva
rumyantseva reviewed Jul 4, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

The idea looks good to me, left a few comments/questions.

internal/backends/sqlite/database.go Outdated Show resolved Hide resolved
integration/basic_test.go Outdated Show resolved Hide resolved
integration/basic_test.go Outdated Show resolved Hide resolved
chilagrow
chilagrow reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Just few things to think about 🤗

integration/basic_test.go Outdated Show resolved Hide resolved
integration/basic_test.go Outdated Show resolved Hide resolved
integration/basic_test.go Outdated Show resolved Hide resolved
integration/basic_test.go Outdated Show resolved Hide resolved
internal/backends/sqlite/database.go Outdated Show resolved Hide resolved
integration/basic_test.go Show resolved Hide resolved
noisersup and others added 2 commits July 5, 2023 15:55
@noisersup
simplify test
f981ce3
@noisersup @chilagrow
Update integration/basic_test.go
6e128c7 
Co-authored-by: Chi Fujii <chi.fujii@ferretdb.io>
AlekSi
AlekSi reviewed Jul 5, 2023
View reviewed changes
integration/basic_test.go Show resolved Hide resolved
integration/basic_test.go Show resolved Hide resolved
integration/basic_test.go Show resolved Hide resolved
internal/handlers/sqlite/msg_ping.go Outdated Show resolved Hide resolved
AlekSi
AlekSi previously approved these changes Jul 5, 2023
View reviewed changes
Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

LGTM overall to avoid blocking that PR. We are good as long as we don't create databases/collections and introduce changes in the contract (that should be completely reworked anyway…). I also think we miss a check for ok in the test, but that's minor

chilagrow
chilagrow reviewed Jul 6, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Just one comment about if we can to test failure case for ping.

rumyantseva
rumyantseva previously approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM

@noisersup noisersup dismissed stale reviews from rumyantseva and AlekSi via cbe7b4f July 6, 2023 09:26
chilagrow
chilagrow previously approved these changes Jul 6, 2023
View reviewed changes
rumyantseva
rumyantseva approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM

chilagrow
chilagrow approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

LGTM

@noisersup noisersup merged commit 040b9ed into FerretDB:main Jul 7, 2023
@noisersup noisersup deleted the ping-sqlite-2756 branch July 7, 2023 15:33
@AlekSi AlekSi added this to the v1.6.0 milestone Jul 17, 2023
@AlekSi AlekSi added code/feature Some user-visible feature is not implemented yet and removed code/chore Code maintenance improvements labels Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chilagrow chilagrow chilagrow approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

@quasilyte quasilyte Awaiting requested review from quasilyte

@AlekSi AlekSi Awaiting requested review from AlekSi

Assignees

@noisersup noisersup

Labels
code/feature Some user-visible feature is not implemented yet
Projects
Current Engineering
Archived in project
Milestone
v1.6.0
Development

Successfully merging this pull request may close these issues.

Implement ping command for SQLite
5 participants
@noisersup @AlekSi @rumyantseva @quasilyte @chilagrow