[feat]curvefs/client:support warmup to disk/mem #2341

Cyber-SiKu · 2023-03-29T06:02:41Z

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

cr-gpt · 2023-03-29T06:02:50Z

curvefs/src/client/common/common.cpp

+    }
+    return ret;
+}
+
 using ::curve::common::StringToUll;

 // if direction is true means '+', false means '-'


.

It looks like the code is written in c++, but there is no include file for c++ library.

The code doesn't check for empty string.

The code doesn't handle any exception if the input string is invalid.

The code should be tested for different inputs and proper error messages should be handled.

The code has a lot of hard coded strings which can be replaced with constants.

The function GetWarmupStorageType() should also have a default return value in case none of the conditions are met.

cr-gpt · 2023-03-29T06:02:54Z

curvefs/src/client/common/common.h

+    kWarmupStorageTypeKvClient = 2,
+};
+
+WarmupStorageType GetWarmupStorageType(const std::string &type);

 enum class FileHandle : uint64_t {
    kDefaultValue = 0,


with code review:

The code looks syntactically correct, however, it could be improved by adding more comments to explain the purpose of each enumeration and function.

It could also be improved by adding input validation to ensure that the type string is a valid type and that the function is returning the correct storage type.

Another improvement would be to make sure that the enum values are consistent across the different enumerations.

cr-gpt · 2023-03-29T06:03:01Z

curvefs/src/client/curve_fuse_op.cpp

    int ret = 0;
    switch (curvefs::client::common::GetWarmupOpType(opTypePath[0])) {
    case curvefs::client::common::WarmupOpType::kWarmupOpAdd:
        ret =
            AddWarmupTask(curvefs::client::common::GetWarmupType(opTypePath[1]),
-                          key, opTypePath[2]);
+                          key, opTypePath[2], storageType);
        if (ret != 0) {
            LOG(ERROR) << name << " has invalid xattr value " << value;
        }


our code review.

The code patch seems to be adding a new argument 'storageType' to two existing functions AddWarmupTask and Warmup. This is good as it increases the flexibility of the functions.

The code patch also checks the validity of the size of the value passed to the Warmup function, which is also a good practice.

The code patch also checks for the storage type of the value passed to the Warmup function, which is a great addition.

There are no obvious bugs in the code, however I would recommend that you add more tests to make sure that the changes are working as expected.

Overall, this looks like a good code patch. Good job!

cr-gpt · 2023-03-29T06:03:08Z

curvefs/src/client/fuse_client.h

        if (fsInfo_->fstype() == FSType::TYPE_S3) {
-            return warmupManager_->AddWarmupFile(key, path);
+            return warmupManager_->AddWarmupFile(key, path, type);
        }  // only support s3
        return true;
    }


!

There are two major changes in the code patch:

Adding optional parameter "type" to PutWarmFilelistTask and PutWarmFileTask functions.

Replacing warmupManager_->AddWarmupFilelist and warmupManager_->AddWarmupFile with warmupManager_->AddWarmupFilelist and warmupManager_->AddWarmupFile respectively.

From the code, it looks like all the parameters are valid and there are no obvious bugs. However, I suggest that you should add some input validation checks for the new parameter 'type', in order to ensure that only valid values are passed. Also, you should add some unit tests to test the functionality of the changes you have made.

cr-gpt · 2023-03-29T06:03:16Z

curvefs/src/client/warmup/warmup_manager.h

-    void PutObjectToCache(const std::string &filename, const char *data,
-                          uint64_t len);
+    void PutObjectToCache(fuse_ino_t key, const std::string &filename,
+                          const char *data, uint64_t len);

 protected:
    std::deque<WarmupFilelist> warmupFilelistDeque_;


the code review

Include proper headers:

The code includes the common.h header which is good, however it should also include the relevant .h files for the used classes like DentryCacheManager, InodeCacheManager and WarmupFile.

Class WarmupProgress:

The constructor declaration should include a default value for the storageType parameter

AddWarmupProcess method:

The method should include a parameter for the type of warmup storage to be used.

AddWarmupFilelist and AddWarmupFile methods:

Both of these methods should include a parameter for the type of warmup storage to be used.

PutObjectToCache method:

The method should include a parameter for the key of the cache entry to be added.

cr-gpt · 2023-03-29T06:03:24Z

tools-v2/pkg/cli/command/curvefs/warmup/add/add.go

@@ -185,7 +198,7 @@ func (aCmd *AddCommand) RunCommand(cmd *cobra.Command, args []string) error {
 		}
 		xattr = CURVEFS_WARMUP_OP_ADD_LIST
 	}
-	value := fmt.Sprintf(xattr, aCmd.CurvefsPath)
+	value := fmt.Sprintf(xattr, aCmd.CurvefsPath, aCmd.StorageType)
 	err := unix.Setxattr(aCmd.Path, CURVEFS_WARMUP_OP_XATTR, []byte(value), 0)
 	if err == unix.ENOTSUP || err == unix.EOPNOTSUPP {
 		return fmt.Errorf("filesystem does not support extended attributes")


with a brief overview of this code patch. Here, a command is being added to add warmup files in a given path to Curvefs. The code creates two constants CURVEFS_WARMUP_OP_XATTR and CURVEFS_WARMUP_OP_ADD_SINGLE/CURVEFS_WARMUP_OP_ADD_LIST which are used to store the values for the xattr and warmup operations. A variable STORAGE_TYPE is also created to store the supported storage types. A struct AddCommand is then defined which stores the necessary data for the command. Finally, the code implements the Init and RunCommand functions for the command which are used to process the input parameters and run the command.

From a code review perspective, I suggest the following changes:

Add more comments to explain the purpose of the code and clarify the logic flow.

Improve error handling by adding checks for invalid input parameters.

Add more tests to ensure that the command behaves as expected.

cr-gpt · 2023-03-29T06:03:30Z

tools-v2/pkg/config/fs.go

+func GetStorageFlag(cmd *cobra.Command) string {
+	return GetFlagString(cmd, CURVEFS_STORAGE)
+}
+
 /* required */

 // copysetid [required]


brief code review:

It looks like the patch code is adding a new flag for storage, is this correct? If yes, it's great to have a brief description of the storage option when calling the command, this can help user understand what type of storage they can choose.

Make sure the default value for storage is valid and reasonable, if the user not provide any input, the system won't crash.

Consider adding more test cases to make sure the new option works correctly.

Cyber-SiKu · 2023-04-06T02:40:10Z

cicheck

Cyber-SiKu · 2023-04-06T06:07:59Z

cicheck

Cyber-SiKu · 2023-04-06T06:37:24Z

cicheck

Cyber-SiKu · 2023-04-06T08:32:16Z

cicheck

Cyber-SiKu · 2023-04-06T08:48:08Z

cicheck

Cyber-SiKu · 2023-04-07T09:41:22Z

cicheck

Cyber-SiKu · 2023-04-10T02:55:15Z

cicheck

Cyber-SiKu · 2023-04-10T07:55:47Z

cicheck

Cyber-SiKu · 2023-04-11T03:05:03Z

cicheck

wu-hanqing · 2023-04-11T08:42:06Z

curvefs/src/client/curve_fuse_op.cpp

@@ -323,16 +324,23 @@ int Warmup(fuse_ino_t key, const std::string& name, const std::string& value) {

    std::vector<std::string> opTypePath;
    curve::common::SplitString(value, "\n", &opTypePath);
-    if (opTypePath.size() != 3) {
+    if (opTypePath.size() != 4) {


it's better to give it a name rather than 4

it's better to give it a name rather than 4

fix

wu-hanqing · 2023-04-11T08:45:54Z

curvefs/src/client/warmup/warmup_manager.cpp

@@ -480,7 +482,7 @@ void WarmupManagerS3Impl::WarmUpAllObjs(
                        VLOG(9) << "no such warmup progress: " << key;
                    }
                }
-                PutObjectToCache(context->key, context->buf, context->len);
+                PutObjectToCache(key, context->key, context->buf, context->len);


Should L482 directly return?

And, since you have found corresponding progress in L478, can we reuse the results? So that, you don't have to search again in PutObjectToCache?

Should L482 directly return?

And, since you have found corresponding progress in L478, can we reuse the results? So that, you don't have to search again in PutObjectToCache?

fix

wu-hanqing · 2023-04-11T08:47:08Z

curvefs/src/client/warmup/warmup_manager.h

        WriteLockGuard lock(inode2ProgressMutex_);
-        auto ret = inode2Progress_.emplace(key, WarmupProgress());
+        auto ret = inode2Progress_.emplace(key, WarmupProgress(0, 0, type));


add another constructor for WarmupProgress that only accept type ?

add another constructor for WarmupProgress that only accept type ?

fix

curvefs/src/client/warmup/warmup_manager.cpp

Cyber-SiKu · 2023-04-11T12:56:28Z

cicheck

Cyber-SiKu · 2023-04-12T03:07:19Z

cicheck

Cyber-SiKu · 2023-04-12T03:37:56Z

cicheck

Cyber-SiKu · 2023-04-12T06:02:16Z

cicheck

Cyber-SiKu · 2023-04-12T09:05:14Z

cicheck

Cyber-SiKu · 2023-04-12T11:54:24Z

cicheck

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

Cyber-SiKu · 2023-04-14T01:52:51Z

cicheck

Cyber-SiKu · 2023-04-14T03:01:19Z

cicheck

Cyber-SiKu · 2023-04-14T03:43:00Z

cicheck

wuhongsong · 2023-04-17T03:11:09Z

curvefs/src/client/common/common.h

+
+enum class WarmupStorageType {
+    kWarmupStorageTypeUnknown = 0,
+    kWarmupStorageTypeDisk = 1,


local + mem cluster maybe clearer?

wuhongsong · 2023-04-17T03:15:28Z

curvefs/src/client/curve_fuse_op.cpp

 int AddWarmupTask(curvefs::client::common::WarmupType type, fuse_ino_t key,
-                  const std::string &path) {
+                  const std::string &path,
+                  curvefs::client::common::WarmupStorageType storageType) {
    int ret = 0;
    bool result = true;
    switch (type) {
    case curvefs::client::common::WarmupType::kWarmupTypeList:
-        result = g_ClientInstance->PutWarmFilelistTask(key);
+        result = g_ClientInstance->PutWarmFilelistTask(key, storageType);


Why not just treat it as a member variable of class？

cr-gpt bot reviewed Mar 29, 2023

View reviewed changes

Cyber-SiKu force-pushed the feat/warmup_disk_memcached branch from a219d78 to 5726aea Compare April 6, 2023 06:07

Cyber-SiKu force-pushed the feat/warmup_disk_memcached branch from 5726aea to 487aeef Compare April 7, 2023 09:37

Cyber-SiKu force-pushed the feat/warmup_disk_memcached branch from 487aeef to 05897ed Compare April 10, 2023 07:55

wu-hanqing reviewed Apr 11, 2023

View reviewed changes

Cyber-SiKu force-pushed the feat/warmup_disk_memcached branch from 05897ed to a3e9652 Compare April 11, 2023 12:56

Cyber-SiKu requested a review from wuhongsong April 13, 2023 02:53

[feat]curvefs/client:support warmup to disk/mem

9e6c299

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

Cyber-SiKu force-pushed the feat/warmup_disk_memcached branch from a3e9652 to 9e6c299 Compare April 13, 2023 11:51

wu-hanqing approved these changes Apr 13, 2023

View reviewed changes

wuhongsong reviewed Apr 17, 2023

View reviewed changes

wuhongsong approved these changes Apr 17, 2023

View reviewed changes

Cyber-SiKu merged commit 6f63fdf into opencurve:master Apr 17, 2023
1 check passed

Cyber-SiKu deleted the feat/warmup_disk_memcached branch April 17, 2023 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat]curvefs/client:support warmup to disk/mem #2341

[feat]curvefs/client:support warmup to disk/mem #2341

Cyber-SiKu commented Mar 29, 2023

cr-gpt bot Mar 29, 2023

cr-gpt bot Mar 29, 2023

cr-gpt bot Mar 29, 2023

cr-gpt bot Mar 29, 2023

cr-gpt bot Mar 29, 2023

cr-gpt bot Mar 29, 2023

cr-gpt bot Mar 29, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 7, 2023

Cyber-SiKu commented Apr 10, 2023

Cyber-SiKu commented Apr 10, 2023

Cyber-SiKu commented Apr 11, 2023

wu-hanqing Apr 11, 2023

Cyber-SiKu Apr 11, 2023

wu-hanqing Apr 11, 2023

Cyber-SiKu Apr 11, 2023

wu-hanqing Apr 11, 2023

Cyber-SiKu Apr 11, 2023

Cyber-SiKu commented Apr 11, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 14, 2023

Cyber-SiKu commented Apr 14, 2023

Cyber-SiKu commented Apr 14, 2023

wuhongsong Apr 17, 2023

wuhongsong Apr 17, 2023

[feat]curvefs/client:support warmup to disk/mem #2341

[feat]curvefs/client:support warmup to disk/mem #2341

Conversation

Cyber-SiKu commented Mar 29, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

cr-gpt bot Mar 29, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 29, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 29, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 29, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 29, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 29, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 29, 2023

Choose a reason for hiding this comment

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 6, 2023

Cyber-SiKu commented Apr 7, 2023

Cyber-SiKu commented Apr 10, 2023

Cyber-SiKu commented Apr 10, 2023

Cyber-SiKu commented Apr 11, 2023

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 Apr 11, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 12, 2023

Cyber-SiKu commented Apr 14, 2023

Cyber-SiKu commented Apr 14, 2023

Cyber-SiKu commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment