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

Implemented response resolver (#1370, #1381) #1381

Merged
merged 11 commits into from
Jan 24, 2023

Conversation

ffuugoo
Copy link
Contributor

@ffuugoo ffuugoo commented Jan 20, 2023

Implemented logic that "merge" multiple Vec<Record>/Vec<ScoredPoint> responses into a single response.

The merged response only retains items that are present either in majority (more than half) or all of the original responses.

TODO:

  • abstract merge logic as a trait, so that it can be used in execute_read_operation
  • add tests

@ffuugoo ffuugoo self-assigned this Jan 20, 2023
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Jan 20, 2023

Just realized there's probably a bug in ScoredPoints merge: merge function assumes points in search/recommend response are sorted by point ids (which is not true).

I'll figure how to fix it soon-ish.

ack @generall

@ffuugoo ffuugoo marked this pull request as ready for review January 23, 2023 19:40
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Jan 23, 2023

This was tedious to implement and annoying to test. @generall, can you make a final review, please?

Copy link
Member

@generall generall left a comment

Choose a reason for hiding this comment

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

@ffuugoo ffuugoo changed the title Implemented aggregation of multiple responses (for the read consistency feature) (#1370) Implemented response resolver (#1370, #1381) Jan 24, 2023
@ffuugoo ffuugoo force-pushed the 1370-read-consistency-response-resolver branch from 9881a70 to b082d52 Compare January 24, 2023 18:49
@ffuugoo ffuugoo merged commit 6b66d33 into dev Jan 24, 2023
@ffuugoo ffuugoo deleted the 1370-read-consistency-response-resolver branch January 24, 2023 19:25
generall added a commit that referenced this pull request Feb 6, 2023
- Implement `Resolve` trait that "merges" multiple `retrieve`/`scroll_by`/`search` responses
  into a single response, to ensure it's consistent across multiple nodes in the cluster
- Implement tests for the `Resolve` trait implementation
- Add a few additional derives required for the `Resolve` trait implementation

Co-authored-by: Andrey Vasnetsov <andrey@vasnetsov.com>
@generall generall mentioned this pull request Apr 19, 2023
8 tasks
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

2 participants