[sharding] Remote shard operations via gRPC #505

agourlay · 2022-04-22T19:11:51Z

This PR enables the remote shards to perform gRPC calls to resole the shard operations (#500)

Most of the code is data type conversion between gRPC types and domain types.

The main design choice is to start with a naive approach where the network channel is recreated for each call.
Channels pooling will be introduced later to reduce the complexity of this PR.

lib/collection/src/operations/conversions.rs

lib/collection/src/shard/remote_shard.rs

lib/segment/src/types.rs

agourlay · 2022-04-26T16:05:27Z

Will need a conflict resolution once #509 is merged as it changes some moving parts regarding the address lookup for peers.

agourlay · 2022-04-26T16:06:17Z

lib/api/src/grpc/conversions.rs

+    fn from(value: segment::types::GeoRadius) -> Self {
+        Self {
+            center: Some(value.center.into()),
+            radius: value.radius as f32, // TODO lossy ok?


do we need to document this loss of precision from f64 to f32 somewhere?

Is it possible to declare them with the same type (e.g. both f64 or both f32)? Or there are some additional limitations?

I believe we use an f32 and not a f64 here because it corresponds to the float in the proto. definition.
https://developers.google.com/protocol-buffers/docs/proto3

Yeah, but they also have double there which is f64 as far as I understand.

Anyway this can be handled in a separate issue, maybe @generall also has some thoughts on this

I agree, I'd rather not change this API contract in the middle of this giant PR 👍

lib/collection/src/operations/conversions.rs

lib/collection/src/shard/remote_shard.rs

agourlay · 2022-04-27T17:47:48Z

Rebased to master to integrate ip_to_address using Uri instead of SocketAddr.

agourlay · 2022-04-28T09:32:41Z

The channel pooling will be introduced later via #518

generall · 2022-04-28T15:58:22Z

lib/storage/src/content_manager/toc.rs

+        #[cfg(not(feature = "consensus"))]
+        {
+            collection
+                .scroll_by(
+                    request,
+                    self.segment_searcher.deref(),
+                    shard_selection,
+                    &HashMap::new(),
+                )
+                .await
+                .map_err(|err| err.into())
+        }


Instead of duplicating everything, can't we just return HashMap::new() from the peer_address_by_id by feature condition?

generall · 2022-04-28T17:20:07Z

lib/collection/src/lib.rs

    }

    pub async fn update_from_client(
        &self,
        operation: CollectionUpdateOperations,
        wait: bool,
+        ip_to_address: &HashMap<u64, Uri>,


I feel like the ip_to_address should be a part of self, otherwise it's a leak of abstraction. As a developer I should not care about cluster state if I want to make an update.

We discussed this choice as well during the design phase with @e-ivkov .

The source of truth is the RaftState on the toc:

raft_state: Arc<std::sync::Mutex<PersistentRaftState>>,

exposed via

pub fn peer_address_by_id(&self) -> Result<PeerAddressById, StorageError> { Ok(self.raft_state.lock()?.peer_address_by_id().clone()) }

Given that the RemoteShard in the collection module does not see any of the RaftState types in the storage, we have decided to go with a more explicit approach propagating only the HashMap at the call site.

What would be you preferred way to encapsulate that state safely in the RemoteShard struct or somewhere else?

The most simple alternative is to have RwLocked or Lock-free shared copy of the peer_address_by_id, which should be updated on changes in raft state.

Alright I will give it a try 👍

I have the change in 9e451ac

As we are not building RemoteShard yet, I left a TODO to mention to share the Arc in the toc.

In a future PR, we could introduce Egor's idea regarding having a read-only lock to prevent the remote shards from modifying the mappings.
I decided to keep it out for now for simplicity.

agourlay · 2022-04-29T13:10:31Z

lib/storage/src/content_manager/raft_state.rs

@@ -55,7 +56,7 @@ impl UnappliedEntries {
 pub struct Persistent {
    state: RaftStateWrapper,
    unapplied_entries: UnappliedEntries,
-    peer_address_by_id: PeerAddressByIdWrapper,
+    peer_address_by_id: Arc<std::sync::RwLock<PeerAddressByIdWrapper>>, // TODO pass to remote shard


the RemoteShard instances are not yet created in the code base so I left a TODO

agourlay · 2022-04-29T13:17:02Z

lib/api/src/grpc/qdrant.rs

@@ -333,7 +333,7 @@ pub mod collections_client {
    where
        T: tonic::client::GrpcService<tonic::body::BoxBody>,
        T::Error: Into<StdError>,
-        T::ResponseBody: Default + Body<Data = Bytes> + Send + 'static,
+        T::ResponseBody: Body<Data = Bytes> + Send + 'static,


the diff in generated code must come from #520

agourlay changed the title ~~Remote shard operations via gRPC~~ WIP [sharding] Remote shard operations via gRPC Apr 22, 2022

e-ivkov reviewed Apr 25, 2022

View reviewed changes

agourlay force-pushed the remote-shard-operation-grpc branch from 3f8b9fe to 5de4cf1 Compare April 25, 2022 12:48

agourlay changed the title ~~WIP [sharding] Remote shard operations via gRPC~~ [sharding] Remote shard operations via gRPC Apr 26, 2022

agourlay marked this pull request as ready for review April 26, 2022 16:02

agourlay commented Apr 26, 2022

View reviewed changes

lib/collection/src/operations/conversions.rs Outdated Show resolved Hide resolved

agourlay requested review from generall and e-ivkov April 26, 2022 18:56

agourlay mentioned this pull request Apr 26, 2022

[sharding] Enable consensus via runtime configuration #511

Closed

agourlay force-pushed the remote-shard-operation-grpc branch from 5d903e6 to f6d4068 Compare April 26, 2022 19:18

e-ivkov reviewed Apr 27, 2022

View reviewed changes

lib/collection/src/shard/remote_shard.rs Outdated Show resolved Hide resolved

agourlay force-pushed the remote-shard-operation-grpc branch 2 times, most recently from fc9d64b to 0df92b9 Compare April 27, 2022 17:45

agourlay force-pushed the remote-shard-operation-grpc branch from 0df92b9 to e1cd954 Compare April 28, 2022 08:14

agourlay mentioned this pull request Apr 28, 2022

[Sharding] Connection pooling for gRPC clients #518

Closed

agourlay linked an issue Apr 28, 2022 that may be closed by this pull request

[sharding] Implement ShardOperation for RemoteShard using gRPC #500

Closed

e-ivkov approved these changes Apr 28, 2022

View reviewed changes

generall requested changes Apr 28, 2022

View reviewed changes

agourlay added 8 commits April 29, 2022 11:05

Remote shard operations via gRPC

f42a7c3

wire up properly the peer id address lookup

6a6ea6a

remove unreachable_code by negating config. predicate

01cdf7e

TryFrom i32 for CollectionStatus

31eb1f7

split conversion code to improve readability

2d15bcc

collect HashMap results

ae90ede

optimize imports

f72b55f

extract request building code for readability

ddd8b2c

ip_to_address uses Uri

cc3456f

agourlay force-pushed the remote-shard-operation-grpc branch from e1cd954 to cc3456f Compare April 29, 2022 09:06

ip_to_address is encapsulated in the RemoteShard

9e451ac

agourlay commented Apr 29, 2022

View reviewed changes

agourlay requested a review from generall April 29, 2022 13:42

generall approved these changes Apr 29, 2022

View reviewed changes

agourlay merged commit 6f93964 into master Apr 29, 2022

agourlay deleted the remote-shard-operation-grpc branch April 29, 2022 17:41

This was referenced Jun 28, 2022

internal point upserts support PointsBatch #756

Merged

Internal upsert points does not support batch #758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sharding] Remote shard operations via gRPC #505

[sharding] Remote shard operations via gRPC #505

agourlay commented Apr 22, 2022 •

edited

Loading

agourlay commented Apr 26, 2022

agourlay Apr 26, 2022

e-ivkov Apr 27, 2022 •

edited

Loading

agourlay Apr 27, 2022

e-ivkov Apr 27, 2022

e-ivkov Apr 27, 2022

agourlay Apr 27, 2022

agourlay commented Apr 27, 2022

agourlay commented Apr 28, 2022

generall Apr 28, 2022

generall Apr 28, 2022

agourlay Apr 28, 2022

generall Apr 29, 2022

agourlay Apr 29, 2022

agourlay Apr 29, 2022

agourlay Apr 29, 2022

agourlay Apr 29, 2022

agourlay Apr 29, 2022

[sharding] Remote shard operations via gRPC #505

[sharding] Remote shard operations via gRPC #505

Conversation

agourlay commented Apr 22, 2022 • edited Loading

agourlay commented Apr 26, 2022

Choose a reason for hiding this comment

e-ivkov Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agourlay commented Apr 27, 2022

agourlay commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agourlay commented Apr 22, 2022 •

edited

Loading

e-ivkov Apr 27, 2022 •

edited

Loading