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

curvefs/client:warmup list and stop #2244

Closed
Cyber-SiKu opened this issue Feb 13, 2023 · 10 comments
Closed

curvefs/client:warmup list and stop #2244

Cyber-SiKu opened this issue Feb 13, 2023 · 10 comments
Assignees
Labels
assigned enhancement improve feature good first issue Good for newcomers pending repairing bug repairing

Comments

@Cyber-SiKu
Copy link
Contributor

Is your feature request related to a problem? (你需要的功能是否与某个问题有关?)

When warming up the list file, it will modify the content of the list file (change the user path to the curvefs path). This change is destructive, and the content of the warmup list file will not be available next time.

Describe the solution you'd like (描述你期望的解决方法)

The tool passes the prefix of the user path and the prefix of the curvefs path as parameters to the client, and then replaces the user path with the curvefs path when warming up.

Describe alternatives you've considered (描述你想到的折衷方案)

Additional context/screenshots (更多上下文/截图)

@Cyber-SiKu Cyber-SiKu added the enhancement improve feature label Feb 13, 2023
@Cyber-SiKu Cyber-SiKu added the good first issue Good for newcomers label Feb 28, 2023
@ashutosh887
Copy link

@Cyber-SiKu Let me work on this

@Cyber-SiKu
Copy link
Contributor Author

@Cyber-SiKu Let me work on this

done

@Cyber-SiKu
Copy link
Contributor Author

@ashutosh887 are you still going on?

@ashutosh887
Copy link

Can you please guide me what are the changes required?
@Cyber-SiKu

@wuhongsong
Copy link
Contributor

Can you please guide me what are the changes required? @Cyber-SiKu

sorry miss it, @Cyber-SiKu have a look

@Cyber-SiKu
Copy link
Contributor Author

@ashutosh887 How this pr implements warmupmanager, you can take a look at this together. If you don't understand something, you can ask questions.

@Cyber-SiKu Cyber-SiKu changed the title curvefs/client:warmup list curvefs/client:warmup list and stop Jun 27, 2023
@ken90242
Copy link
Contributor

ken90242 commented Jul 23, 2023

Hi @Cyber-SiKu, @wuhongsong

I am interested in this task, and I would love to work on it.

Below, I'll present my solution; please let me know if it makes sense to you:

1. Ensure every path in the file list is valid

  • [NEW] add.go/verifyFilelist - create a new function verifyFilelist, which reuses most of the convertFilelist function (except for the path replacement part)
  • [DEL] add.go/convertFilelist delete it since no one else will use it anymore
  • [MODIFY] add.go/RunCommand - verify the filelist with the newly created function: verifyFilelist when we know it is a warm-up add filelist operation

2. Pass the prefix of the user path and curvefs path to the client

  • [MOD] add.go/RunCommand - append two more parameters to value: mountpoint.Root, mountpoint.MountPoint

3. Extract the prefix of the user path and curvefs path from the passed-in value

  • [MOD] curve_fuse_op.cpp/Warmup - extract mountpoint.Root and mountpoint.MountPoint from values[4], values[5]
  • [MOD] common.h/kWarmupOpNum - change the number from 4 to 6

4. Forward mountpoint.Root and mountpoint.MountPoint all the way to where they will be used

  • [MOD] curve_fuse_op.cpp/Warmup - include mountpoint.Root and mountpoint.MountPoint as parameters when calling AddWarmupTask

  • [MOD] curve_fuse_op.cpp/AddWarmupTask - change its signature to receive mountpoint.Root and mountpoint.MountPoint

  • [MOD] curve_fuse_op.cpp/AddWarmupTask - include mountpoint.Root and mountpoint.MountPoint as parameters when calling g_ClientInstance->PutWarmFilelistTask

  • [MOD] fuse_client.h/PutWarmFilelistTask - change its signature to receive mountpoint.Root and mountpoint.MountPoint

  • [MOD] fuse_client.h/PutWarmFilelistTask - include mountpoint.Root and mountpoint.MountPoint as parameters when calling warmupManager_->AddWarmupFilelist

  • [MOD] warmup_manager.cpp/AddWarmupFilelist - change its signature to receive mountpoint.Root and mountpoint.MountPoint

  • [MOD] warmup_manager.cpp/AddWarmupFilelist - include mountpoint.Root and mountpoint.MountPoint as parameters when calling warmupFilelistDeque_.emplace_back

  • [MOD] warmup_manager.h/WarmupFile - create two new members: mountpoint.Root and mountpoint.MountPoint for WarmupFile class, and update WarmupFile’s constructor

5. Replace the user path with the curvefs path

  • [MOD] warmup_manager.cpp/GetWarmupList - extract mountpoint.Root and mountpoint.MountPoint from filelist, then update each line of file by replacing the mountpoint.MountPoint with mountpoint.Root

6. Verify the change is valid

(I couldn't find any existing test cases related to functions I will change,
so I suppose there are no relevant test cases available at the moment?)

  • Manually run the tool to see if the update works

@Cyber-SiKu
Copy link
Contributor Author

Hi @Cyber-SiKu, @wuhongsong

I am interested in this task, and I would love to work on it.

Below, I'll present my solution; please let me know if it makes sense to you:

1. Ensure every path in the file list is valid

  • [NEW] add.go/verifyFilelist - create a new function verifyFilelist, which reuses most of the convertFilelist function (except for the path replacement part)
  • [DEL] add.go/convertFilelist delete it since no one else will use it anymore
  • [MODIFY] add.go/RunCommand - verify the filelist with the newly created function: verifyFilelist when we know it is a warm-up add filelist operation

2. Pass the prefix of the user path and curvefs path to the client

  • [MOD] add.go/RunCommand - append two more parameters to value: mountpoint.Root, mountpoint.MountPoint

3. Extract the prefix of the user path and curvefs path from the passed-in value

  • [MOD] curve_fuse_op.cpp/Warmup - extract mountpoint.Root and mountpoint.MountPoint from values[4], values[5]
  • [MOD] common.h/kWarmupOpNum - change the number from 4 to 6

4. Forward mountpoint.Root and mountpoint.MountPoint all the way to where they will be used

  • [MOD] curve_fuse_op.cpp/Warmup - include mountpoint.Root and mountpoint.MountPoint as parameters when calling AddWarmupTask
  • [MOD] curve_fuse_op.cpp/AddWarmupTask - change its signature to receive mountpoint.Root and mountpoint.MountPoint
  • [MOD] curve_fuse_op.cpp/AddWarmupTask - include mountpoint.Root and mountpoint.MountPoint as parameters when calling g_ClientInstance->PutWarmFilelistTask
  • [MOD] fuse_client.h/PutWarmFilelistTask - change its signature to receive mountpoint.Root and mountpoint.MountPoint
  • [MOD] fuse_client.h/PutWarmFilelistTask - include mountpoint.Root and mountpoint.MountPoint as parameters when calling warmupManager_->AddWarmupFilelist
  • [MOD] warmup_manager.cpp/AddWarmupFilelist - change its signature to receive mountpoint.Root and mountpoint.MountPoint
  • [MOD] warmup_manager.cpp/AddWarmupFilelist - include mountpoint.Root and mountpoint.MountPoint as parameters when calling warmupFilelistDeque_.emplace_back
  • [MOD] warmup_manager.h/WarmupFile - create two new members: mountpoint.Root and mountpoint.MountPoint for WarmupFile class, and update WarmupFile’s constructor

5. Replace the user path with the curvefs path

  • [MOD] warmup_manager.cpp/GetWarmupList - extract mountpoint.Root and mountpoint.MountPoint from filelist, then update each line of file by replacing the mountpoint.MountPoint with mountpoint.Root

6. Verify the change is valid

(I couldn't find any existing test cases related to functions I will change,
so I suppose there are no relevant test cases available at the moment?)

  • Manually run the tool to see if the update works

good job!
Indeed, it is far more fruitful to implement your ideas. Pass the root and mountpoint to the client as parameters. This can effectively prevent tools from modifying files.

@ken90242
Copy link
Contributor

ken90242 commented Aug 8, 2023

@Cyber-SiKu

It seems that I have overlooked the functionalities of warmup list and warmup stop lol.
Could you please provide the specific requirements for these? Once I have those details, I will present my proposal asap.

@caoxianfei1
Copy link
Contributor

@Cyber-SiKu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned enhancement improve feature good first issue Good for newcomers pending repairing bug repairing
Projects
None yet
Development

No branches or pull requests

5 participants