[feat]curvefs/client kvcache monitor #2806

Cyber-SiKu · 2023-10-16T08:36:48Z

fix base metric(bps/qps/latnecy) error
add hit and miss metric
change metric prefix to curvefs_kvclient_manager
add memcache client metric

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 · 2023-10-16T08:42:23Z

cicheck

Cyber-SiKu · 2023-10-16T09:07:00Z

cicheck

wu-hanqing · 2023-10-16T09:15:39Z

curvefs/src/client/kvclient/kvclient_manager.cpp

@@ -5,7 +5,7 @@
 *  you may not use this file except in compliance with the License.
 *  You may obtain a copy of the License at
 *
- *      http://www.apache.org/licenses/LICENSE-2.0
+ *      http://www.apache.org/licenses/LICENSE-2.0https://godbolt.org/


wu-hanqing · 2023-10-16T09:28:53Z

curvefs/src/client/kvclient/kvclient_manager.cpp

+        case MEMCACHED_DATA_DOES_NOT_EXIST:
+            // The data requested with the key given was not found.
+        case MEMCACHED_DATA_EXISTS:


same meaning of exists and not exist?

same meaning of exists and not exist?

https://awesomized.github.io/libmemcached/libmemcached/memcached_return_t.html#_CPPv4N18memcached_return_t21MEMCACHED_DATA_EXISTSE

it's interesting ⁉️

wu-hanqing · 2023-10-16T09:30:13Z

curvefs/src/client/kvclient/kvclient_manager.cpp

    client_ = kvclient;
+    kvClientManagerMetric_ = std::make_shared<KVClientManagerMetric>(fsName);


unique_ptr?

unique_ptr?

https://github.com/opencurve/curve/blob/6d07ba2e01de5a96b1d77b4635b25c7bfc7a7e87/curvefs/src/client/kvclient/kvclient_manager.h#L117-L119

wu-hanqing · 2023-10-16T09:35:55Z

curvefs/src/client/kvclient/kvclient_manager.cpp

-
-        task->done(task);
+        if (task->res) {
+            kvClientManagerMetric_->count << 1;


Now that there are hits and misses, is count redundant?

Now that there are hits and misses, is count redundant?

count means set

hits and miss are get

wu-hanqing · 2023-10-16T09:36:48Z

curvefs/src/client/kvclient/kvclient_manager.cpp

 void KVClientManager::Get(std::shared_ptr<GetKVCacheTask> task) {
    threadPool_.Enqueue([task, this]() {
-        LatencyGuard guard(&kvClientMetric_.kvClientGet.latency);
+        LatencyGuard guard(&kvClientManagerMetric_->get.latency);


should we only update metric for susuccessful request?

wu-hanqing · 2023-10-16T09:38:11Z

curvefs/src/client/kvclient/kvclient_manager.h

@@ -104,15 +114,17 @@ class KVClientManager {

    void Get(std::shared_ptr<GetKVCacheTask> task);


why shared_ptr?

why shared_ptr?

i don't known

wu-hanqing · 2023-10-16T09:39:14Z

curvefs/src/client/metric/client_metric.h

+    InterfaceMetric get;
+    InterfaceMetric set;


what's the difference between KVClientManagerMetric's get/set ?

what's the difference between KVClientManagerMetric's get/set ?

manager will put set task to queue.

this write to memcache

SeanHai · 2023-10-16T10:53:22Z

curvefs/src/client/kvclient/kvclient_manager.h

@@ -71,7 +72,8 @@ struct GetKVCacheTask {
    const std::string& key;
    char* value;
    uint64_t offset;
-    uint64_t length;
+    uint64_t valueLength;
+    uint64_t length;  // actual length of value


what's the difference?

what's the difference?

One is the length of value, and the other is the length of the object obtained from kvcache.

SeanHai · 2023-10-16T11:04:01Z

What's the difference of latency of kvclientmanager and memcachedClient ?

SeanHai · 2023-10-16T11:05:32Z

Are the units of set latency in the kvcache manager panel correct?

Cyber-SiKu · 2023-10-16T12:19:49Z

What's the difference of latency of kvclientmanager and memcachedClient ?

kvCachemanager is used to manage memcacheclient. Because kvcache uses a thread pool for asynchronous IO, and memcacheclient uses synchronous IO. So separating the two shows how long io waits in the queue.

Cyber-SiKu · 2023-10-16T12:21:13Z

Are the units of set latency in the kvcache manager panel correct?

fix by pr#2808

Cyber-SiKu · 2023-10-17T06:18:14Z

cicheck

Cyber-SiKu · 2023-10-17T12:12:46Z

cicheck

Cyber-SiKu · 2023-10-18T01:55:05Z

cicheck

Cyber-SiKu · 2023-10-18T03:31:44Z

cicheck

Cyber-SiKu · 2023-10-19T02:34:44Z

cicheck

Cyber-SiKu · 2023-10-19T06:35:18Z

cicheck

Cyber-SiKu · 2023-10-19T06:36:30Z

cicheck

Cyber-SiKu · 2023-10-19T06:53:57Z

cicheck

Cyber-SiKu · 2023-10-19T07:12:09Z

cicheck

Cyber-SiKu · 2023-10-19T07:12:21Z

cicheck

Cyber-SiKu · 2023-10-19T10:07:31Z

cicheck

Cyber-SiKu · 2023-10-19T10:07:49Z

cicheck

Cyber-SiKu · 2023-10-19T11:36:02Z

cicheck

Cyber-SiKu · 2023-10-20T02:18:11Z

cicheck

Cyber-SiKu · 2023-10-20T02:48:32Z

cicheck

Cyber-SiKu · 2023-10-20T02:51:06Z

cicheck

Cyber-SiKu · 2023-10-20T03:38:26Z

cicheck

Cyber-SiKu · 2023-10-20T03:43:31Z

cicheck

Cyber-SiKu · 2023-10-21T10:58:52Z

cicheck

Cyber-SiKu · 2023-10-21T10:59:15Z

cicheck

Cyber-SiKu · 2023-10-21T10:59:24Z

cicheck

Cyber-SiKu · 2023-10-21T11:01:13Z

cicheck

1. fix base metric(bps/qps/latnecy) error 2. add hit and miss metric 3. change metric prefix to curvefs_kvclient_manager 4. add memcache client metric Signed-off-by: Cyber-SiKu <Cyber-SiKu@outlook.com>

Cyber-SiKu · 2023-10-21T13:04:54Z

cicheck

Cyber-SiKu · 2023-10-21T13:05:40Z

cicheck

Cyber-SiKu · 2023-10-21T13:05:54Z

cicheck

Cyber-SiKu · 2023-10-21T13:06:06Z

cicheck

Cyber-SiKu · 2023-10-21T13:06:18Z

cicheck

Cyber-SiKu · 2023-10-21T13:06:43Z

cicheck

Cyber-SiKu force-pushed the feat/curvefs/client/kvcache_monitor branch 2 times, most recently from bac2a5f to 64f0099 Compare October 16, 2023 08:41

Cyber-SiKu force-pushed the feat/curvefs/client/kvcache_monitor branch from 64f0099 to d452499 Compare October 16, 2023 09:06

wu-hanqing reviewed Oct 16, 2023

View reviewed changes

Cyber-SiKu force-pushed the feat/curvefs/client/kvcache_monitor branch 3 times, most recently from 81d0be4 to cb1a6a5 Compare October 16, 2023 10:27

SeanHai reviewed Oct 16, 2023

View reviewed changes

Cyber-SiKu force-pushed the feat/curvefs/client/kvcache_monitor branch from cb1a6a5 to 8cc7ab1 Compare October 16, 2023 12:35

Cyber-SiKu closed this Oct 17, 2023

Cyber-SiKu reopened this Oct 17, 2023

Cyber-SiKu force-pushed the feat/curvefs/client/kvcache_monitor branch from 8cc7ab1 to 4cc32e4 Compare October 17, 2023 12:12

Cyber-SiKu closed this Oct 18, 2023

Cyber-SiKu reopened this Oct 18, 2023

Cyber-SiKu force-pushed the feat/curvefs/client/kvcache_monitor branch from 4cc32e4 to 4b31b8e Compare October 19, 2023 02:18

Cyber-SiKu closed this Oct 19, 2023

Cyber-SiKu reopened this Oct 19, 2023

wu-hanqing approved these changes Oct 19, 2023

View reviewed changes

SeanHai approved these changes Oct 20, 2023

View reviewed changes

[feat]curvefs/client kvcache monitor

5e44c53

1. fix base metric(bps/qps/latnecy) error 2. add hit and miss metric 3. change metric prefix to curvefs_kvclient_manager 4. add memcache client metric Signed-off-by: Cyber-SiKu <Cyber-SiKu@outlook.com>

Cyber-SiKu force-pushed the feat/curvefs/client/kvcache_monitor branch from 4b31b8e to 5e44c53 Compare October 21, 2023 13:03

Cyber-SiKu closed this Oct 21, 2023

Cyber-SiKu reopened this Oct 21, 2023

Cyber-SiKu merged commit cc5095a into opencurve:master Oct 23, 2023
7 checks passed

Cyber-SiKu deleted the feat/curvefs/client/kvcache_monitor branch October 23, 2023 01:52

		client_ = kvclient;
		kvClientManagerMetric_ = std::make_shared<KVClientManagerMetric>(fsName);

		@@ -104,15 +114,17 @@ class KVClientManager {

		void Get(std::shared_ptr<GetKVCacheTask> task);

[feat]curvefs/client kvcache monitor #2806

[feat]curvefs/client kvcache monitor #2806

Conversation

Cyber-SiKu commented Oct 16, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Cyber-SiKu commented Oct 16, 2023

Cyber-SiKu commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cyber-SiKu Oct 16, 2023 • edited

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

Cyber-SiKu Oct 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanHai commented Oct 16, 2023

SeanHai commented Oct 16, 2023

Cyber-SiKu commented Oct 16, 2023 • edited

Cyber-SiKu commented Oct 16, 2023

Cyber-SiKu commented Oct 17, 2023

Cyber-SiKu commented Oct 17, 2023

Cyber-SiKu commented Oct 18, 2023

Cyber-SiKu commented Oct 18, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 19, 2023

Cyber-SiKu commented Oct 20, 2023

Cyber-SiKu commented Oct 20, 2023

Cyber-SiKu commented Oct 20, 2023

Cyber-SiKu commented Oct 20, 2023

Cyber-SiKu commented Oct 20, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu commented Oct 21, 2023

Cyber-SiKu Oct 16, 2023 •

edited

Cyber-SiKu Oct 16, 2023 •

edited

Cyber-SiKu commented Oct 16, 2023 •

edited