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

Improve the name mechanism of vineyard objects. #2010

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions python/client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,18 @@ void bind_client(py::module& mod) {
},
"object_id"_a, py::arg("wait") = false,
py::call_guard<py::gil_scoped_release>())
.def(
"name_exists",
[](ClientBase* self, std::string const& name) -> bool {
ObjectID object_id;
auto status = self->GetName(name, object_id, false);
if (status.ok()) {
return true;
} else {
return false;
}
},
"name"_a)
.def(
"drop_name",
[](ClientBase* self, std::string const& name) {
Expand Down
14 changes: 14 additions & 0 deletions python/pybind11_docs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,20 @@ Get the associated object id of the given name.
ObjectID: The associated object id with the name.
)doc";

const char* ClientBase_name_exists = R"doc(
.. method:: name_exists(name: str or ObjectName) -> bool
:noindex:

Check if the given name exists in the vineyard server.

Parameters:
name: str
The name that will be checked.

Returns:
bool: :code:`True` if the name exists in the vineyard server.
)doc";

const char* ClientBase_list_names = R"doc(
.. method:: list_names(pattern: str, regex: bool = False, limit: int = 5)
-> Dict[str, ObjectID]
Expand Down
5 changes: 4 additions & 1 deletion python/vineyard/core/builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def put(
builder: Optional[BuilderContext] = None,
persist: bool = False,
name: Optional[str] = None,
**kwargs
**kwargs,
):
"""Put python value to vineyard.

Expand Down Expand Up @@ -191,6 +191,9 @@ def put(
Returns:
ObjectID: The result object id will be returned.
"""
if name is not None and client.name_exists(name):
raise ValueError(f"Name {name} already exists in the vineyard")

if builder is not None:
return builder(client, value, **kwargs)

Expand Down
14 changes: 12 additions & 2 deletions python/vineyard/core/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,12 +370,18 @@ def create_metadata(
@_apply_docstring(IPCClient.delete)
def delete(
self,
object: Union[ObjectID, Object, ObjectMeta, List[ObjectID]],
object_id: Union[ObjectID, Object, ObjectMeta, List[ObjectID]] = None,
name: str = None,
force: bool = False,
deep: bool = True,
memory_trim: bool = False,
) -> None:
return self.default_client().delete(object, force, deep, memory_trim)
if object_id is None and name is None:
raise ValueError("Either object_id or name should be provided.")
if name is not None:
object_id = self.default_client().get_name(name)
self.default_client().drop_name(name)
return self.default_client().delete(object_id, force, deep, memory_trim)

@_apply_docstring(IPCClient.create_stream)
def create_stream(self, id: ObjectID) -> None:
Expand Down Expand Up @@ -439,6 +445,10 @@ def put_name(self, object: Union[Object, ObjectMeta, ObjectID], name: str) -> No
def get_name(self, name: str, wait: bool = False) -> ObjectID:
return self.default_client().get_name(name, wait)

@_apply_docstring(IPCClient.name_exists)
def name_exists(self, name: str) -> bool:
return self.default_client().name_exists(name)

@_apply_docstring(IPCClient.drop_name)
def drop_name(self, name: str) -> None:
return self.default_client().drop_name(name)
Expand Down
46 changes: 38 additions & 8 deletions src/server/server/vineyard_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,23 @@ namespace vineyard {
} while (0)
#endif // ENSURE_VINEYARDD_READY

bool is_all_digits(const std::string& name) {
for (char ch : name) {
if (!std::isdigit(static_cast<unsigned char>(ch))) {
return false;
}
}
return true;
}

// Helper function to adjust the name if it's all digits.
std::string AdjustName(const std::string& name) {
if (is_all_digits(name)) {
return "_" + name;
}
return name;
}

bool DeferredReq::Alive() const { return alive_fn_(); }

bool DeferredReq::TestThenCall(const json& meta) const {
Expand Down Expand Up @@ -871,13 +888,22 @@ Status VineyardServer::PutName(const ObjectID object_id,
const std::string& name, callback_t<> callback) {
ENSURE_VINEYARDD_READY();
auto self(shared_from_this());
// if the name is all digits, prefix it with '_'
// as the nlohmann/json can't convert '/names/1234: 12345' to 'names:{"1234",
// "12345"}'
std::string new_name = AdjustName(name);

meta_service_ptr_->RequestToPersist(
[object_id, name](const Status& status, const json& meta,
std::vector<meta_tree::op_t>& ops) {
[object_id, name = new_name](const Status& status, const json& meta,
std::vector<meta_tree::op_t>& ops) {
if (status.ok()) {
// TODO: do proper validation:
// 1. global objects can have name, local ones cannot.
// 2. the name-object_id mapping shouldn't be overwrite.

auto names = meta.value("names", json(nullptr));
if (names.is_object() && names.contains(name)) {
return Status::ObjectExists("name " + name + " already exists");
}

// blob cannot have name
if (IsBlob(object_id)) {
Expand Down Expand Up @@ -928,9 +954,11 @@ Status VineyardServer::GetName(const std::string& name, const bool wait,
callback_t<const ObjectID&> callback) {
ENSURE_VINEYARDD_READY();
auto self(shared_from_this());
meta_service_ptr_->RequestToGetData(true, [self, name, wait, alive, callback](
const Status& status,
const json& meta) {
std::string new_name = AdjustName(name);

meta_service_ptr_->RequestToGetData(true, [self, name = new_name, wait, alive,
callback](const Status& status,
const json& meta) {
if (status.ok()) {
auto test_task = [name](const json& meta) -> bool {
auto names = meta.value("names", json(nullptr));
Expand Down Expand Up @@ -967,10 +995,12 @@ Status VineyardServer::GetName(const std::string& name, const bool wait,
Status VineyardServer::DropName(const std::string& name,
callback_t<> callback) {
ENSURE_VINEYARDD_READY();
std::string new_name = AdjustName(name);

auto self(shared_from_this());
meta_service_ptr_->RequestToPersist(
[name](const Status& status, const json& meta,
std::vector<meta_tree::op_t>& ops) {
[name = new_name](const Status& status, const json& meta,
std::vector<meta_tree::op_t>& ops) {
if (status.ok()) {
auto names = meta.value("names", json(nullptr));
if (names.is_object()) {
Expand Down
Loading