new: convert cluster operations to grpc#1116
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds bidirectional conversion between gRPC and REST representations for cluster-level operations and updates synchronous and asynchronous remote clients to use these conversions when gRPC is preferred. For gRPC calls the clients convert incoming cluster operations to gRPC messages and populate the corresponding field on UpdateCollectionClusterSetupRequest (move_shard, replicate_shard, abort_transfer, drop_replica, create_shard_key, delete_shard_key, restart_transfer, replicate_points). REST code paths remain unchanged. New test fixtures and a parameterized test cover both prefer_grpc options. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (3)qdrant_client/qdrant_remote.py (3)
qdrant_client/conversions/conversion.py (2)
qdrant_client/async_qdrant_remote.py (4)
🪛 Ruff (0.14.5)qdrant_client/qdrant_remote.py2612-2612: Avoid specifying long messages outside the exception class (TRY003) qdrant_client/conversions/conversion.py2109-2111: Avoid specifying long messages outside the exception class (TRY003) 2171-2171: Avoid specifying long messages outside the exception class (TRY003) 2191-2193: Avoid specifying long messages outside the exception class (TRY003) 2208-2210: Avoid specifying long messages outside the exception class (TRY003) 2236-2238: Avoid specifying long messages outside the exception class (TRY003) 2270-2270: Avoid specifying long messages outside the exception class (TRY003) 4594-4594: Prefer (TRY004) 4594-4594: Avoid specifying long messages outside the exception class (TRY003) 4597-4597: Prefer (TRY004) 4597-4597: Avoid specifying long messages outside the exception class (TRY003) 4599-4599: Avoid specifying long messages outside the exception class (TRY003) 4692-4692: Avoid specifying long messages outside the exception class (TRY003) qdrant_client/async_qdrant_remote.py2425-2425: Avoid specifying long messages outside the exception class (TRY003) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
qdrant_client/conversions/conversion.py (2)
2123-2270: Cluster op gRPC→REST conversions look correct; consider guarding MoveShard.to_shard_id as wellThe new
GrpcToRest.convert_cluster_operationsand per-op converters (convert_move_shard,convert_replicate_shard,convert_abort_shard_transfer,convert_replica,convert_create_shard_key,convert_delete_shard_key,convert_restart_transfer,convert_replicate_points,convert_shard_transfer_method) are structurally sound and line up with the HTTP models and grpc stubs:
- Field mappings (ids, peer ids, method, filter, shard keys, replica state) match the referenced model definitions.
- Internal-only fields like
to_shard_idare explicitly rejected forReplicateShard,AbortShardTransfer, andRestartTransfer, which avoids silently losing information that has no REST representation.- Shard transfer method mapping is symmetric with the REST→gRPC converter.
The one inconsistency is
convert_move_shard: it doesn’t guard againstmodel.HasField("to_shard_id"), while the other cluster ops do. If core ever setsto_shard_idon aMoveShard, that information would be silently dropped on the REST side, unlike the explicitValueErrorbehavior for the other operations.If you want consistent behavior across all cluster ops, you could add a similar guard:
@classmethod def convert_move_shard(cls, model: grpc.MoveShard) -> rest.MoveShard: + if model.HasField("to_shard_id"): + raise ValueError( + "to_shard_id is a field for internal purposes, can't be converted to rest" + ) # pragma: no cover return rest.MoveShard( shard_id=model.shard_id, from_peer_id=model.from_peer_id, to_peer_id=model.to_peer_id, method=cls.convert_shard_transfer_method(model.method) if model.HasField("method") else None, )This matches the defensive approach already used for
ReplicateShard,AbortShardTransfer, andRestartTransfer.
2254-2270: ShardTransferMethod conversions are symmetric; minor stylistic nit onlyThe new
convert_shard_transfer_methodimplementations in both directions correctly cover the four documented methods (stream_records,snapshot,wal_delta,resharding_stream_records) and are symmetric betweenGrpcToRestandRestToGrpc.The only minor nit is the inconsistent style of referring to gRPC enums (
grpc.StreamRecords/grpc.ReshardingStreamRecordsvsgrpc.ShardTransferMethod.Snapshot/WalDelta). That’s purely cosmetic and doesn’t affect behavior.If you want to make this a bit clearer for future readers, you could normalize to the enum-qualified style everywhere (e.g.
grpc.ShardTransferMethod.StreamRecords), but it’s not required.Also applies to: 4675-4690
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
qdrant_client/async_qdrant_remote.py(1 hunks)qdrant_client/conversions/conversion.py(3 hunks)qdrant_client/qdrant_remote.py(1 hunks)tests/conversions/fixtures.py(2 hunks)tests/test_qdrant_client.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/conversions/fixtures.py (2)
qdrant_client/grpc/collections_pb2.pyi (26)
move_shard(2906-2906)MoveShard(2669-2697)ShardTransferMethod(330-330)replicate_shard(2908-2908)ReplicateShard(2701-2729)AbortShardTransfer(2733-2755)restart_transfer(2918-2918)RestartTransfer(2759-2784)replicate_points(2920-2920)ReplicatePoints(2788-2812)from_shard_key(2795-2796)ShardKey(2495-2512)to_shard_key(2798-2799)filter(2801-2802)Replica(2816-2829)create_shard_key(2914-2914)CreateShardKey(2833-2869)shard_key(2530-2531)shard_key(2560-2561)shard_key(2614-2614)shard_key(2842-2843)shard_key(2878-2879)placement(2849-2850)ReplicaState(269-269)delete_shard_key(2916-2916)DeleteShardKey(2873-2886)qdrant_client/http/models/models.py (9)
MoveShard(1666-1672)ShardTransferMethod(2827-2838)ReplicateShard(2410-2416)AbortShardTransfer(22-25)RestartTransfer(2447-2451)ReplicatePoints(2400-2403)Filter(914-924)Replica(2366-2368)ReplicaState(2380-2397)
qdrant_client/qdrant_remote.py (2)
qdrant_client/conversions/conversion.py (2)
convert_cluster_operations(2124-2171)convert_cluster_operations(4549-4597)qdrant_client/grpc/collections_pb2.pyi (9)
MoveShard(2669-2697)ReplicateShard(2701-2729)AbortShardTransfer(2733-2755)Replica(2816-2829)CreateShardKey(2833-2869)DeleteShardKey(2873-2886)RestartTransfer(2759-2784)ReplicatePoints(2788-2812)UpdateCollectionClusterSetupRequest(2890-2942)
qdrant_client/async_qdrant_remote.py (3)
qdrant_client/conversions/conversion.py (2)
convert_cluster_operations(2124-2171)convert_cluster_operations(4549-4597)qdrant_client/grpc/collections_pb2.pyi (9)
MoveShard(2669-2697)ReplicateShard(2701-2729)AbortShardTransfer(2733-2755)Replica(2816-2829)CreateShardKey(2833-2869)DeleteShardKey(2873-2886)RestartTransfer(2759-2784)ReplicatePoints(2788-2812)UpdateCollectionClusterSetupRequest(2890-2942)qdrant_client/grpc/collections_service_pb2_grpc.py (6)
CreateShardKey(175-181)CreateShardKey(457-471)DeleteShardKey(183-189)DeleteShardKey(474-488)UpdateCollectionClusterSetup(167-173)UpdateCollectionClusterSetup(440-454)
tests/test_qdrant_client.py (3)
qdrant_client/qdrant_client.py (1)
upsert(874-953)qdrant_client/http/models/models.py (2)
ShardKeyWithFallback(2779-2781)PointStruct(1985-1988)qdrant_client/qdrant_remote.py (1)
upsert(1048-1138)
qdrant_client/conversions/conversion.py (2)
qdrant_client/grpc/collections_pb2.pyi (24)
MoveShard(2669-2697)ReplicateShard(2701-2729)AbortShardTransfer(2733-2755)Replica(2816-2829)CreateShardKey(2833-2869)DeleteShardKey(2873-2886)RestartTransfer(2759-2784)ReplicatePoints(2788-2812)move_shard(2906-2906)replicate_shard(2908-2908)abort_transfer(2910-2910)drop_replica(2912-2912)restart_transfer(2918-2918)replicate_points(2920-2920)shard_key(2530-2531)shard_key(2560-2561)shard_key(2614-2614)shard_key(2842-2843)shard_key(2878-2879)placement(2849-2850)filter(2801-2802)from_shard_key(2795-2796)to_shard_key(2798-2799)ShardTransferMethod(330-330)qdrant_client/http/models/models.py (19)
MoveShard(1666-1672)ReplicateShard(2410-2416)AbortShardTransfer(22-25)Replica(2366-2368)RestartTransfer(2447-2451)ReplicatePoints(2400-2403)MoveShardOperation(1675-1676)ReplicateShardOperation(2419-2420)AbortTransferOperation(28-29)DropReplicaOperation(798-799)CreateShardingKeyOperation(579-580)DropShardingKeyOperation(806-807)RestartTransferOperation(2454-2455)ReplicatePointsOperation(2406-2407)CreateShardingKey(559-576)DropShardingKey(802-803)ShardTransferMethod(2827-2838)StartReshardingOperation(3056-3057)AbortReshardingOperation(18-19)
🪛 GitHub Actions: type-checkers
qdrant_client/conversions/conversion.py
[error] 4549-4549: Mypy error: Missing return statement at qdrant_client/conversions/conversion.py:4549. Command failed: 'poetry run mypy . --exclude "tools/async_client_generator" --disallow-incomplete-defs --disallow-untyped-defs' (exit code 1).
🪛 Ruff (0.14.5)
qdrant_client/qdrant_remote.py
2614-2614: Avoid specifying long messages outside the exception class
(TRY003)
qdrant_client/async_qdrant_remote.py
2427-2427: Avoid specifying long messages outside the exception class
(TRY003)
qdrant_client/conversions/conversion.py
2109-2111: Avoid specifying long messages outside the exception class
(TRY003)
2171-2171: Avoid specifying long messages outside the exception class
(TRY003)
2191-2193: Avoid specifying long messages outside the exception class
(TRY003)
2208-2210: Avoid specifying long messages outside the exception class
(TRY003)
2236-2238: Avoid specifying long messages outside the exception class
(TRY003)
2270-2270: Avoid specifying long messages outside the exception class
(TRY003)
4594-4594: Prefer TypeError exception for invalid type
(TRY004)
4594-4594: Avoid specifying long messages outside the exception class
(TRY003)
4597-4597: Prefer TypeError exception for invalid type
(TRY004)
4597-4597: Avoid specifying long messages outside the exception class
(TRY003)
4690-4690: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
🔇 Additional comments (3)
tests/test_qdrant_client.py (1)
2267-2336: Cluster collection update test cleanly exercises both REST and gRPC pathsThis parametrized
test_cluster_collection_updatedoes a nice job of covering the new cluster operations end‑to‑end:
- Runs against both
prefer_grpc=FalseandTrue, so it validates the new conversion paths in both directions.- Exercises creation of shard keys (including an initial
ReplicaState.PARTIAL), upsert usingShardKeyWithFallback, a filteredReplicatePointsOperation, and finally dropping a shard key.The structure and version gating are consistent with the rest of this test module.
tests/conversions/fixtures.py (2)
1701-1732: Well-organized fixture mappings for cluster operations.The dictionary structure effectively supports parameterized testing with:
- A comprehensive "ClusterOperations" grouping for general conversion testing
- Individual type-specific entries for targeted testing of each operation
This organization aligns well with the existing fixture patterns in the file.
1486-1486: No changes required — fixture design is intentional and correct.The
restart_transferfixture at line 1486 is intentionally created without themethodparameter as part of parametrized testing. This follows the established pattern used for similar cluster operations (move_shardandreplicate_shard), which also provide two test variants each: one with an explicit method value and one relying on the proto3 default.In proto3, all fields are implicitly optional, and unset enum fields default to 0 (the first enum value,
StreamRecords). The conversion functions handle this correctly by directly accessingmodel.methodwithout checks, allowing the implicit default to propagate. This design enables testing both explicit and default-value scenarios during REST↔gRPC conversions.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
qdrant_client/conversions/conversion.py (1)
4548-4600: Past review concern resolved.The previous review flagged that this method could fall through without returning for unknown
ClusterOperationsvariants. This has been properly addressed with the addition of the finalraise ValueErrorat line 4599, ensuring the function never returnsNoneand provides a clear error for unsupported cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
qdrant_client/async_qdrant_remote.py(1 hunks)qdrant_client/conversions/conversion.py(3 hunks)qdrant_client/qdrant_remote.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
qdrant_client/qdrant_remote.py (3)
qdrant_client/conversions/conversion.py (2)
convert_cluster_operations(2124-2171)convert_cluster_operations(4549-4599)qdrant_client/grpc/collections_pb2.pyi (9)
MoveShard(2669-2697)ReplicateShard(2701-2729)AbortShardTransfer(2733-2755)Replica(2816-2829)CreateShardKey(2833-2869)DeleteShardKey(2873-2886)RestartTransfer(2759-2784)ReplicatePoints(2788-2812)UpdateCollectionClusterSetupRequest(2890-2942)qdrant_client/grpc/collections_service_pb2_grpc.py (6)
CreateShardKey(175-181)CreateShardKey(457-471)DeleteShardKey(183-189)DeleteShardKey(474-488)UpdateCollectionClusterSetup(167-173)UpdateCollectionClusterSetup(440-454)
qdrant_client/async_qdrant_remote.py (2)
qdrant_client/conversions/conversion.py (2)
convert_cluster_operations(2124-2171)convert_cluster_operations(4549-4599)qdrant_client/grpc/collections_pb2.pyi (9)
MoveShard(2669-2697)ReplicateShard(2701-2729)AbortShardTransfer(2733-2755)Replica(2816-2829)CreateShardKey(2833-2869)DeleteShardKey(2873-2886)RestartTransfer(2759-2784)ReplicatePoints(2788-2812)UpdateCollectionClusterSetupRequest(2890-2942)
qdrant_client/conversions/conversion.py (2)
qdrant_client/grpc/collections_pb2.pyi (24)
MoveShard(2669-2697)ReplicateShard(2701-2729)AbortShardTransfer(2733-2755)Replica(2816-2829)CreateShardKey(2833-2869)DeleteShardKey(2873-2886)RestartTransfer(2759-2784)ReplicatePoints(2788-2812)move_shard(2906-2906)replicate_shard(2908-2908)abort_transfer(2910-2910)drop_replica(2912-2912)restart_transfer(2918-2918)replicate_points(2920-2920)shard_key(2530-2531)shard_key(2560-2561)shard_key(2614-2614)shard_key(2842-2843)shard_key(2878-2879)placement(2849-2850)filter(2801-2802)from_shard_key(2795-2796)to_shard_key(2798-2799)ShardTransferMethod(330-330)qdrant_client/http/models/models.py (19)
MoveShard(1666-1672)ReplicateShard(2410-2416)AbortShardTransfer(22-25)Replica(2366-2368)RestartTransfer(2447-2451)ReplicatePoints(2400-2403)MoveShardOperation(1675-1676)ReplicateShardOperation(2419-2420)AbortTransferOperation(28-29)DropReplicaOperation(798-799)CreateShardingKeyOperation(579-580)DropShardingKeyOperation(806-807)RestartTransferOperation(2454-2455)ReplicatePointsOperation(2406-2407)CreateShardingKey(559-576)DropShardingKey(802-803)ShardTransferMethod(2827-2838)StartReshardingOperation(3056-3057)AbortReshardingOperation(18-19)
🪛 Ruff (0.14.5)
qdrant_client/qdrant_remote.py
2612-2612: Avoid specifying long messages outside the exception class
(TRY003)
qdrant_client/async_qdrant_remote.py
2425-2425: Avoid specifying long messages outside the exception class
(TRY003)
qdrant_client/conversions/conversion.py
2109-2111: Avoid specifying long messages outside the exception class
(TRY003)
2171-2171: Avoid specifying long messages outside the exception class
(TRY003)
2191-2193: Avoid specifying long messages outside the exception class
(TRY003)
2208-2210: Avoid specifying long messages outside the exception class
(TRY003)
2236-2238: Avoid specifying long messages outside the exception class
(TRY003)
2270-2270: Avoid specifying long messages outside the exception class
(TRY003)
4594-4594: Prefer TypeError exception for invalid type
(TRY004)
4594-4594: Avoid specifying long messages outside the exception class
(TRY003)
4597-4597: Prefer TypeError exception for invalid type
(TRY004)
4597-4597: Avoid specifying long messages outside the exception class
(TRY003)
4599-4599: Avoid specifying long messages outside the exception class
(TRY003)
4692-4692: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
tbung
left a comment
tbung
left a comment
There was a problem hiding this comment.
A bunch of this code could probably benefit from match statements when we drop 3.9, some of it might be slightly faster as look up tables (I didn't check what is and isn't hashable tho, so this might not work)
These are just some ideas/nits, nothing that needs to be addressed.
| if model == grpc.ShardTransferMethod.StreamRecords: | ||
| return rest.ShardTransferMethod.STREAM_RECORDS | ||
|
|
||
| if model == grpc.ShardTransferMethod.Snapshot: | ||
| return rest.ShardTransferMethod.SNAPSHOT | ||
|
|
||
| if model == grpc.ShardTransferMethod.WalDelta: | ||
| return rest.ShardTransferMethod.WAL_DELTA | ||
|
|
||
| if model == grpc.ShardTransferMethod.ReshardingStreamRecords: | ||
| return rest.ShardTransferMethod.RESHARDING_STREAM_RECORDS |
There was a problem hiding this comment.
Nit: Could stuff like this not be a look up table?
| if isinstance(model, rest.MoveShardOperation): | ||
| operation = model.move_shard | ||
| return cls.convert_move_shard(operation) | ||
|
|
||
| if isinstance(model, rest.ReplicateShardOperation): | ||
| operation = model.replicate_shard | ||
| return cls.convert_replicate_shard(operation) | ||
|
|
||
| if isinstance(model, rest.AbortTransferOperation): | ||
| operation = model.abort_transfer | ||
| return cls.convert_abort_shard_transfer(operation) | ||
|
|
||
| if isinstance(model, rest.DropReplicaOperation): | ||
| operation = model.drop_replica | ||
| return cls.convert_replica(operation) | ||
|
|
||
| if isinstance(model, rest.CreateShardingKeyOperation): | ||
| operation = model.create_sharding_key | ||
| return cls.convert_create_shard_key(operation) | ||
|
|
||
| if isinstance(model, rest.DropShardingKeyOperation): | ||
| operation = model.drop_sharding_key | ||
| return cls.convert_delete_shard_key(operation) | ||
|
|
||
| if isinstance(model, rest.RestartTransferOperation): | ||
| operation = model.restart_transfer | ||
| return cls.convert_restart_transfer(operation) | ||
|
|
||
| if isinstance(model, rest.ReplicatePointsOperation): | ||
| operation = model.replicate_points | ||
| return cls.convert_replicate_points(operation) | ||
|
|
||
| if isinstance(model, rest.StartReshardingOperation): # pragma: no cover | ||
| raise ValueError("StartReshardingOperation has no grpc counterpart") | ||
|
|
||
| if isinstance(model, rest.AbortReshardingOperation): # pragma: no cover | ||
| raise ValueError("AbortReshardingOperation has not grpc counterpart") |
There was a problem hiding this comment.
If not look up table, this is probably nicer as a match statement once we drop 3.9
There was a problem hiding this comment.
I guess we can update it later, yes
* new: convert cluster operations to grpc * fix: rollback coverage html * fix: address ai comments * fix: address review comments
2 participants
No description provided.