From 6135ff0346683064868a9b2aa4c9422cb06e7273 Mon Sep 17 00:00:00 2001 From: Ye Cao Date: Thu, 31 Oct 2024 14:15:37 +0800 Subject: [PATCH] Improve the name mechanism of vineyard objects. * Make sure one name is associated with one vineyard object. * Fix the name can't be all digits. * Support delete vineyard object by name. Signed-off-by: Ye Cao --- python/client.cc | 12 +++++++++ python/pybind11_docs.cc | 14 +++++++++++ python/vineyard/core/builder.py | 5 +++- python/vineyard/core/client.py | 14 +++++++++-- src/server/server/vineyard_server.cc | 37 ++++++++++++++++++++++------ 5 files changed, 71 insertions(+), 11 deletions(-) diff --git a/python/client.cc b/python/client.cc index 6f0f5fc62..afc5cc42a 100644 --- a/python/client.cc +++ b/python/client.cc @@ -487,6 +487,18 @@ void bind_client(py::module& mod) { }, "object_id"_a, py::arg("wait") = false, py::call_guard()) + .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) { diff --git a/python/pybind11_docs.cc b/python/pybind11_docs.cc index 792b08386..77691c5eb 100644 --- a/python/pybind11_docs.cc +++ b/python/pybind11_docs.cc @@ -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] diff --git a/python/vineyard/core/builder.py b/python/vineyard/core/builder.py index 36cc46bb1..8bd6b8b8c 100644 --- a/python/vineyard/core/builder.py +++ b/python/vineyard/core/builder.py @@ -156,7 +156,7 @@ def put( builder: Optional[BuilderContext] = None, persist: bool = False, name: Optional[str] = None, - **kwargs + **kwargs, ): """Put python value to vineyard. @@ -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) diff --git a/python/vineyard/core/client.py b/python/vineyard/core/client.py index 2bb0b1c2f..42d58605e 100644 --- a/python/vineyard/core/client.py +++ b/python/vineyard/core/client.py @@ -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: @@ -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) diff --git a/src/server/server/vineyard_server.cc b/src/server/server/vineyard_server.cc index 982f8d538..1fcb24d9e 100644 --- a/src/server/server/vineyard_server.cc +++ b/src/server/server/vineyard_server.cc @@ -57,6 +57,14 @@ namespace vineyard { } while (0) #endif // ENSURE_VINEYARDD_READY +// Helper function to adjust the name if it's all digits. +std::string AdjustName(const std::string& name) { + if (std::regex_match(name, std::regex("[0-9]+$"))) { + return "_" + name; + } + return name; +} + bool DeferredReq::Alive() const { return alive_fn_(); } bool DeferredReq::TestThenCall(const json& meta) const { @@ -871,13 +879,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); + std::cout << "new_name is: " << new_name << std::endl; meta_service_ptr_->RequestToPersist( - [object_id, name](const Status& status, const json& meta, - std::vector& ops) { + [object_id, name = new_name](const Status& status, const json& meta, + std::vector& 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)) { @@ -928,9 +945,11 @@ Status VineyardServer::GetName(const std::string& name, const bool wait, callback_t 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)); @@ -967,10 +986,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& ops) { + [name = new_name](const Status& status, const json& meta, + std::vector& ops) { if (status.ok()) { auto names = meta.value("names", json(nullptr)); if (names.is_object()) {