curvefs: support memcache cluster #2108

Cyber-SiKu · 2022-11-29T07:52:33Z

add define in topology.proto
add RegistMemcacheCluster in topology service
add define in topology_item
add AllocOrGetMemcacheCluster in client/mdsclinet
add ListMemcacheCluster in topology service

Signed-off-by: Cyber-SiKu Cyber-SiKu@outlook.com

What problem does this PR solve?

Issue Number: #xxx

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

Relevant documentation/comments is changed or added
I acknowledge that all my contributions will be made under the project's license

Cyber-SiKu · 2022-11-29T08:33:59Z

recheck

Cyber-SiKu · 2022-11-29T11:18:12Z

recheck

Cyber-SiKu · 2022-11-30T08:32:06Z

recheck

Cyber-SiKu · 2022-12-02T07:03:30Z

recheck

Cyber-SiKu · 2022-12-05T03:24:19Z

recheck

Cyber-SiKu · 2022-12-05T05:37:05Z

recheck

Cyber-SiKu · 2022-12-06T02:09:27Z

recheck

Cyber-SiKu · 2022-12-07T03:13:56Z

recheck

cw123 · 2022-12-07T11:00:49Z

curvefs/proto/topology.proto

+message ListMemcacheClusterRequest {
+}
+
+message MemcacheClusterInfo {


the message is not same with #2096

the message is not same with #2096

#2096 Merge first, fix when merge.

cw123 · 2022-12-07T11:00:59Z

curvefs/proto/topology.proto

@@ -458,6 +459,33 @@ message ListTopologyResponse {
    required ListMetaServerResponse metaservers = 5;
 }

+message MemcacheServerInfo {


the message is not same with #2096

cw123 · 2022-12-07T11:03:09Z

curvefs/src/client/rpcclient/mds_client.cpp

@@ -413,6 +413,40 @@ bool MdsClientImpl::ListPartition(uint32_t fsID,
    return 0 == rpcexcutor_.DoRPCTask(task, mdsOpt_.mdsMaxRetryMS);
 }

+bool MdsClientImpl::AllocOrGetMemcacheCluster(MemcacheClusterInfo* cluster) {
+    auto task = RPCTask {


add metric for this

add metric for this

fix

cw123 · 2022-12-07T11:05:06Z

curvefs/src/client/rpcclient/mds_client.cpp

+            response.memclusters();
+        if (!clusters.empty()) {
+            int randId =
+                (static_cast<int>(butil::fast_rand()) % clusters.size()) + 1;


why +1 here

why +1 here

fix

cw123 · 2022-12-07T11:09:09Z

curvefs/src/mds/topology/topology_item.h

@@ -57,7 +58,7 @@ struct ClusterInformation {
    std::map<uint32_t, uint32_t> partitionIndexs;

    ClusterInformation() = default;
-    explicit ClusterInformation(const std::string &clusterId)
+    explicit ClusterInformation(const std::string& clusterId)


这个文件有大量的格式修改，没有办法review，这种仅仅是格式的修改，而且旧的格式问题不大，最好不要去动他，建议格式恢复回去再review。

This file has a lot of format changes, and there is no way to review it. This is only a format modification, and the old format is not a big problem. It is best not to touch it. It is recommended to restore the format and review it.

这个文件有大量的格式修改，没有办法review，这种仅仅是格式的修改，而且旧的格式问题不大，最好不要去动他，建议格式恢复回去再review。

This file has a lot of format changes, and there is no way to review it. This is only a format modification, and the old format is not a big problem. It is best not to touch it. It is recommended to restore the format and review it.

fix

Cyber-SiKu · 2022-12-08T05:37:33Z

recheck

SeanHai · 2022-12-08T03:02:30Z

curvefs/src/client/rpcclient/mds_client.cpp

+            return ret;
+        }
+
+        *cluster = response.cluster();


*cluster = std::move(*response.mutable_cluster()) ?

don't use response.cluster() instead with *response.mutable_cluster()

SeanHai · 2022-12-08T03:25:43Z

curvefs/src/mds/topology/topology_manager.cpp

+    response->set_statuscode(TopoStatusCode::TOPO_OK);
+    // register memcacheCluster as server
+    curve::common::NameLockGuard lock(registMemcacheClusterMutex_,
+                                      request->ShortDebugString());


I'm not sure the ShortDebugString are the same when the different order of servers added in request.

I'm not sure the ShortDebugString are the same when the different order of servers added in request.

fix

SeanHai · 2022-12-08T03:47:25Z

curvefs/src/mds/topology/topology_manager.cpp

+                                      request->ShortDebugString());
+
+    // Guarantee the uniqueness of memcacheServer
+    std::list<MemcacheServer> serverRegisted = topology_->ListMemcacheServers();


Can one machine serve two clusters?

Can one machine serve two clusters?

yep

SeanHai · 2022-12-08T07:14:55Z

curvefs/src/mds/topology/topology_manager.cpp

+void TopologyManager::ListMemcacheCluster(
+    ListMemcacheClusterResponse* response) {
+    std::list<MemcacheCluster> clusterList = topology_->ListMemcacheClusters();
+    if (clusterList.empty()) {


if (!clusterList.empty())

if (!clusterList.empty())

fix

cw123 · 2022-12-08T10:37:28Z

curvefs/src/mds/topology/topology_storge_etcd.h

+
+    bool StorageFs2MemCluster(FsIdType fsId,
+                              MemcacheClusterIdType memClusterId) override;
+    bool LoadFs2MemCluster(std::unordered_map<FsIdType, MemcacheClusterIdType>*


MemCluster is MemcacheCluster

memCluster may be mistaken for memoryCluster

MemCluster is MemcacheCluster

memCluster may be mistaken for memoryCluster

fix

cw123 · 2022-12-08T10:48:05Z

curvefs/src/mds/topology/topology_manager.cpp

+}
+
+void TopologyManager::ListMemcacheCluster(
+    ListMemcacheClusterResponse* response) {


add readlock registMemcacheClusterMutex_

add readlock registMemcacheClusterMutex_

In topology_->ListMemcacheClusters(); there is ReadLockGuard rlockMemcacheCluster(memcacheClusterMutex_); which can already guarantee the validity of the data?

cw123 · 2022-12-08T10:48:24Z

curvefs/src/mds/topology/topology_manager.cpp

+
+void TopologyManager::AllocOrGetMemcacheCluster(
+    const AllocOrGetMemcacheClusterRequest* request,
+    AllocOrGetMemcacheClusterResponse* response) {


add readlock registMemcacheClusterMutex_

add readlock registMemcacheClusterMutex_

ditto
topology_->AllocOrGetMemcacheCluster to ensure data consistency.

cw123 · 2022-12-08T10:54:06Z

curvefs/src/mds/topology/topology.cpp

+        memClusterMap_[data.GetId()] = data;
+        memClusterMap_.insert(std::make_pair(data.GetId(), std::move(data)));


two line is dup

two line is dup

fix

cw123 · 2022-12-08T10:57:04Z

curvefs/src/mds/topology/topology.cpp

+        int randId =
+            static_cast<int>(butil::fast_rand()) % memClusterMap_.size();
+        auto iter = memClusterMap_.cbegin();
+        for (int i = 0; i <= randId; ++i, ++iter) continue;


for condition is wrong

for condition is wrong

fix

YunhuiChen · 2022-12-09T02:39:20Z

recheck

SeanHai · 2022-12-09T03:23:59Z

create a new issue related this pr about add unit tests of this pr.

Cyber-SiKu · 2022-12-09T05:51:20Z

create a new issue related this pr about add unit tests of this pr.

add #2147

Cyber-SiKu · 2022-12-09T08:05:44Z

cicheck

1. add define in topology.proto 2. add RegistMemcacheCluster in topology service 3. add define in topology_item 4. add AllocOrGetMemcacheCluster in client/mdsclinet 5. add ListMemcacheCluster in topology service Signed-off-by: Cyber-SiKu <Cyber-SiKu@outlook.com>

Cyber-SiKu · 2022-12-09T08:10:35Z

cicheck

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch from f6e0e81 to a9d7a6f Compare November 29, 2022 08:06

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch from a9d7a6f to 7dd30cc Compare November 29, 2022 09:13

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch from 7dd30cc to bc28a61 Compare November 29, 2022 11:43

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch 2 times, most recently from 09f70af to 27aa16b Compare December 1, 2022 07:03

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch from 27aa16b to 2963e9d Compare December 5, 2022 02:53

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch 2 times, most recently from 266d12e to c334fdd Compare December 7, 2022 03:11

Cyber-SiKu requested review from SeanHai and cw123 December 7, 2022 03:17

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch from c334fdd to 11dcba6 Compare December 7, 2022 05:10

cw123 reviewed Dec 7, 2022

View reviewed changes

ilixiaocui mentioned this pull request Dec 7, 2022

Memcache Support #2096

Merged

2 tasks

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch 2 times, most recently from 081cc60 to 2a3aa95 Compare December 8, 2022 02:23

SeanHai reviewed Dec 8, 2022

View reviewed changes

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch 2 times, most recently from 79affe4 to bd311cd Compare December 8, 2022 09:22

cw123 reviewed Dec 8, 2022

View reviewed changes

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch 3 times, most recently from c3682be to b4ce25f Compare December 9, 2022 02:22

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch from b4ce25f to 706dd2a Compare December 9, 2022 03:05

cw123 approved these changes Dec 9, 2022

View reviewed changes

SeanHai approved these changes Dec 9, 2022

View reviewed changes

Cyber-SiKu force-pushed the curvefs/mds_add_memchache_in_topology branch from 706dd2a to 63f4d15 Compare December 9, 2022 08:09

Cyber-SiKu merged commit 982b864 into opencurve:master Dec 9, 2022

Cyber-SiKu deleted the curvefs/mds_add_memchache_in_topology branch December 9, 2022 09:20

		memClusterMap_[data.GetId()] = data;
		memClusterMap_.insert(std::make_pair(data.GetId(), std::move(data)));

curvefs: support memcache cluster #2108

curvefs: support memcache cluster #2108

Conversation

Cyber-SiKu commented Nov 29, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Cyber-SiKu commented Nov 29, 2022

Cyber-SiKu commented Nov 29, 2022

Cyber-SiKu commented Nov 30, 2022

Cyber-SiKu commented Dec 2, 2022

Cyber-SiKu commented Dec 5, 2022

Cyber-SiKu commented Dec 5, 2022

Cyber-SiKu commented Dec 6, 2022

Cyber-SiKu commented Dec 7, 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

Cyber-SiKu commented Dec 8, 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

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

Choose a reason for hiding this comment

YunhuiChen commented Dec 9, 2022

SeanHai commented Dec 9, 2022

Cyber-SiKu commented Dec 9, 2022

Cyber-SiKu commented Dec 9, 2022

Cyber-SiKu commented Dec 9, 2022