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 a wrapper to manage IPC and RPC clients and revise the statement about RPC client in documentation #1629

Merged
merged 12 commits into from
Dec 13, 2023
Merged

Add a wrapper to manage IPC and RPC clients and revise the statement about RPC client in documentation #1629

merged 12 commits into from
Dec 13, 2023

Conversation

dashanji
Copy link
Member

@dashanji dashanji commented Dec 1, 2023 •
edited
Loading

What do these changes do?

  • Update the doc for rpc client.
  • Add the client wrapper to handle the IPCClient and RPCClient.

sighingnow
sighingnow requested changes Dec 1, 2023
View reviewed changes
python/vineyard/core/client_manager.py Outdated
from vineyard._C import _connect


class ClientManager:
Copy link
Member

Choose a reason for hiding this comment

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

ClientManager -> Client.

You could rename the existing Client to IPCClient (only in python/client.cc).

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@dashanji dashanji changed the title Update the doc of rpc client and add client manager for getting objects [WIP] Update the doc of rpc client and add client manager for getting objects Dec 1, 2023
@dashanji dashanji force-pushed the add-client-manager branch 2 times, most recently from 0c2aaf0 to 1c0593f Compare December 6, 2023 08:00
@dashanji dashanji changed the title [WIP] Update the doc of rpc client and add client manager for getting objects Update the doc of rpc client and add client manager for handling the IPCClient and RPCClient Dec 6, 2023
@dashanji dashanji requested a review from sighingnow December 6, 2023 08:03
sighingnow
sighingnow requested changes Dec 12, 2023
View reviewed changes
Copy link
Member

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

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

Please address these "Either all return statements in a function should return an expression, or none of them should." warnings.

python/vineyard/core/tests/test_client_on_distributed_vineyard.py Outdated
@@ -0,0 +1,81 @@
#! /usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

I cannot see if there are any cases that can be covered by this test file but cannot be covered by existing test cases.

Copy link
Member Author

@dashanji dashanji Dec 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, the main purpose of this test is to get the same data from different vineyard nodes with different clients.

ipc client1 put vineyard instance 1/ vineyard instance 2/ vineyard instance 3 -> obj1/obj2/obj3
rpc client1 put vineyard instance 1/ vineyard instance 2/ vineyard instance 3 -> obj4/obj5/obj6

test:
rpc client1 get obj1/obj2/obj3 == obj1/obj2/obj3
ipc client1 get obj4/obj5/obj6 == obj4/obj5/obj6

Copy link
Member

Choose a reason for hiding this comment

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

Move the test case to https://github.com/v6d-io/v6d/blob/main/python/vineyard/deploy/tests/test_distributed.py

python/vineyard/core/resolver.py Outdated

if resolver is None:
resolver = get_current_resolvers()
return resolver(obj, __vineyard_client=client, **kw)


setattr(IPCClient, 'get', get)
setattr(RPCClient, 'get', get)
Copy link
Member

Choose a reason for hiding this comment

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

Keep the original bindings as well.

python/vineyard/core/client.py Outdated
"Failed to resolve IPC socket or RPC endpoint of vineyard server from "
"environment variables VINEYARD_IPC_SOCKET or VINEYARD_RPC_ENDPOINT."
)
return self
Copy link
Member

Choose a reason for hiding this comment

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

Please put contents in connect() to __init__() to make it more Pythonic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@dashanji dashanji force-pushed the add-client-manager branch 3 times, most recently from f994a3d to 4e16f22 Compare December 12, 2023 09:21
@sighingnow sighingnow changed the title Update the doc of rpc client and add client manager for handling the IPCClient and RPCClient Add a wrapper to manage IPC and RPC clients and revise the statement about RPC client in documentation Dec 13, 2023
@sighingnow
Fixes argument resolution
738d47a 
Signed-off-by: Tao He <[email protected]>
@sighingnow
Fixes
4dc1b2f 
Signed-off-by: Tao He <[email protected]>
@sighingnow sighingnow merged commit 76c823a into v6d-io:main Dec 13, 2023
4 of 5 checks passed
@sighingnow sighingnow deleted the add-client-manager branch December 13, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sighingnow sighingnow sighingnow requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dashanji @sighingnow