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

Add read_consistency parameter to the APIs (#1371) #1407

Merged
merged 30 commits into from
Feb 3, 2023
Merged

Add read_consistency parameter to the APIs (#1371) #1407

merged 30 commits into from
Feb 3, 2023

Conversation

ffuugoo
Copy link
Contributor

@ffuugoo ffuugoo commented Jan 26, 2023 •
edited
Loading

Add read_consistency parameter to the HTTP and gRPC APIs.

TODO:

  • add/update documentation

@ffuugoo ffuugoo requested a review from generall January 26, 2023 20:32
@ffuugoo ffuugoo self-assigned this Jan 26, 2023
ffuugoo
ffuugoo commented Jan 26, 2023
View reviewed changes
Copy link
Contributor Author

@ffuugoo ffuugoo left a comment

Choose a reason for hiding this comment

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

@generall I need a bit of help here.

src/actix/api/retrieve_api.rs Outdated Show resolved Hide resolved
lib/collection/src/recommendations.rs Outdated Show resolved Hide resolved
lib/collection/src/collection.rs Outdated Show resolved Hide resolved
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Jan 27, 2023

OMG, the PR grew to 500 lines, just as WriteOrdering one did. 🙈

@ffuugoo
Copy link
Contributor Author

ffuugoo commented Jan 27, 2023 •
edited
Loading

For some reason ReadConsistency::Factor deserialization from query parameter does not work in HTTP API. :/

E.g.:

  • curl 'http://127.0.0.1:6333/collections/test_collection/points/1?read_consistency=all' works just fine
  • but curl 'http://127.0.0.1:6333/collections/test_collection/points/1?read_consistency=2' returns Query deserialize error: data did not match any variant of untagged enum ReadConsistency error
  • all while the test in consistency_params.rs pass just fine

I'll figure it out tomorrow.

Other than this issue, I only need to run some local tests in clustered mode, and it's done.

ack @generall

@ffuugoo ffuugoo force-pushed the 1371-add-read-consistency-param-to-api branch from d3dd470 to 885a017 Compare January 30, 2023 19:33
@ffuugoo ffuugoo marked this pull request as ready for review January 30, 2023 19:34
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Jan 30, 2023 •
edited
Loading

@generall Done. Fixed the issue with deserialization, fixed the bug with payload comparison, rebased on latest dev. I've also run a #1408 test with read consistency enabled. I've had to add additional sleep after killing uploader processes (see my comment in the #1408), however, with the read consistency enabled the test pass 10 times out of 10 attempts, while it fails without the feature on some of the runs.

ffuugoo and others added 16 commits January 31, 2023 15:24
@generall generall force-pushed the 1371-add-read-consistency-param-to-api branch from 487c83f to 0c6261f Compare January 31, 2023 14:25
@ffuugoo ffuugoo force-pushed the 1371-add-read-consistency-param-to-api branch from a1afdcb to 66da667 Compare January 31, 2023 17:21
@ffuugoo
fixup! Fix tests
4a99dd5 
Gotta love those negative conditions, or how a missed `!` can ruin your day... 🤦‍♀️
@ffuugoo
Make internal API calls strictly "local-shard only"
25378e6
@ffuugoo
Implement a few basic traits for ResolverRecord
2f0b20d
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Feb 1, 2023

@generall A single character typo (that I've introduced myself, of course) caused The Bug. Should have found it sooner, as everything was hinting me in the right direction... but, well, most bugs seem obvious in retrospect. :/

generall
generall reviewed Feb 1, 2023
View reviewed changes
lib/segment/src/types.rs Outdated Show resolved Hide resolved
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Feb 2, 2023

@generall Can you, please, take a final look at the PR?

@generall generall merged commit a50e7bd into dev Feb 3, 2023
generall added a commit that referenced this pull request Feb 6, 2023
@ffuugoo @generall
Add read_consistency parameter to the APIs (#1371) (#1407)
34bdbc2 
* WIP: Add `read_consistency` parameter to the APIs

* WIP: Add `read_consistency` parameter to the APIs

TODO:
- Add documentation

* `cargo fmt`

* Add gRPC documentation

* Add OpenAPI documentation

* Cleanup

* fixup! Add OpenAPI documentation

* fixup! Add gRPC documentation

Who would have known there's `generate_grpc_docs.sh`!? 🥲🙈🤦‍♀️

* generate openapi

* Fix `read_consistency` query parameter deserialization

* Further improve `read_consistency` query parameter deserialization

* `cargo clippy`

* Fix `Payload` comparison during read operation result resolving

* Fix grammar

* rename `read_consistency` -> `consistency` and add integration test

* use majority for test

* fix tests

* Fix tests

* fixup! Fix tests

Apply the same fix to `ScoredPoint`

* Remove an `unwrap`

* fixup! Fix tests

Gotta love those negative conditions, or how a missed `!` can ruin your day... 🤦‍♀️

* Make internal API calls strictly "local-shard only"

* Implement a few basic traits for `ResolverRecord`

* fixup! Implement a few basic traits for `ResolverRecord`

* Revert "Make internal API calls strictly "local-shard only""

This reverts commit 25378e6.

* Fix `Record::payload` and `ScoredPoint::payload` serialization

* Revert "Fix `Record::payload` and `ScoredPoint::payload` serialization"

This reverts commit b566bea.

* Fix `Record::payload` and `ScoredPoint::payload` visibility

* fixup! Fix `Record::payload` and `ScoredPoint::payload` visibility

Remove `todo!()`

* refactoring

---------

Co-authored-by: Andrey Vasnetsov <andrey@vasnetsov.com>
@ffuugoo ffuugoo deleted the 1371-add-read-consistency-param-to-api branch March 3, 2023 11:19
@generall generall mentioned this pull request Apr 19, 2023
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@generall generall generall approved these changes

@agourlay agourlay agourlay left review comments

Assignees

@ffuugoo ffuugoo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ffuugoo @agourlay @generall