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

Feature: sync all snapshot data in Pika process without rsync subprocess #1805

Merged
merged 44 commits into from
Jul 26, 2023
Merged

Conversation

wangshao1
Copy link
Collaborator

fix issue #1228 ,这个版本的全量同步有如下 feature:

1 断点续传
2 速率限制
3 文件检验

wangshaoyi and others added 30 commits July 7, 2023 19:11
implement rsync network transfer

Co-authored-by: wangshaoyi <[email protected]>
* fix action
* implement rsync network transfer
add rsyncclient to syncslaveslot
* add read meta file and data
* fix bug

* add rsyncclient to syncslaveslot

* fix compile error

* fix compile error

---------

Co-authored-by: wangshaoyi <[email protected]>
rsyncclient periodically flush meta table
* change rsync response
* fix pika rsync bug
* add debug log for test

* fix rsync client/server bugs

* fix bugs

* add debug log for test

* fix bugs

* fix bugs

* fix bugs

* fix bugs

* fix bugs

* fix bugs 1

* fix bugs

---------

Co-authored-by: wangshaoyi <[email protected]>
include/pika_define.h Outdated Show resolved Hide resolved
src/rsync_server.cc Outdated Show resolved Hide resolved
src/rsync_server.cc Outdated Show resolved Hide resolved
* fix by review comments
Co-authored-by: wangshaoyi <[email protected]>

} //end namespace rsync
#endif

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided code patch appears to be a header file related to a client thread for an rsync application. Here are some suggestions and areas to consider in your code review:

  1. Licensing: Verify that the license information provided (BSD-style license) is accurate and consistent with your project's requirements.

  2. Namespace Usage: It seems that the namespaces pstd and net are being used in this file. Ensure that these namespaces are correctly defined and included in the relevant source files.

  3. Class Design: Review the design of the RsyncClientConn, RsyncClientConnFactory, and RsyncClientThread classes to ensure they align with your intended functionality. Consider whether any additional member variables or methods are needed or if any existing ones can be made more specific.

  4. Error Handling: Check if proper error handling mechanisms, such as exception handling or error return codes, are implemented in the code. It is crucial to handle potential errors or exceptional scenarios gracefully.

  5. Documentation: Consider adding appropriate comments/documentation to aid understanding and maintainability of the codebase. Include brief descriptions of the purpose of classes, methods, and important variables.

  6. Thread Safety: If multiple threads will access the code in question, validate that proper synchronization mechanisms, like locks or atomic operations, are used where necessary to prevent race conditions.

  7. Memory Management: Examine how memory allocation and deallocation are handled within your code. Ensure there are no memory leaks or undefined behavior and that destructors and smart pointers (if applicable) are used appropriately.

  8. Code Style and Consistency: Verify that the coding style is consistent throughout the codebase, adhering to any existing conventions. Maintain readability by following best practices such as indentation, meaningful variable names, and appropriate use of white space.

Remember that a comprehensive code review requires access to the complete codebase including related implementation files, so these suggestions may not cover all aspects of your project.

* fix by review comments
---------

Co-authored-by: wangshaoyi <[email protected]>
@@ -26,6 +26,7 @@
#include "include/pika_cmd_table_manager.h"
#include "include/pika_dispatch_thread.h"
#include "include/pika_rm.h"
#include "pstd_hash.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个往下挪动下,很明显他不在 #include "include/pika_xx.h" 这块 include 代码块里面

src/pika_slot.cc Outdated
@@ -142,6 +143,7 @@ void Slot::PrepareRsync() {
// 3, Update master offset, and the PikaAuxiliaryThread cron will connect and do slaveof task with master
bool Slot::TryUpdateMasterOffset() {
std::string info_path = dbsync_path_ + kBgsaveInfoFile;
// todo 这里要改动,定期向 master 发送 meta_rsync 的请求
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

改成英文 comment

src/pika_slot.cc Outdated
if (snapshot_uuid_.empty()) {
std::string info_data;
const std::string infoPath = bgsave_info().path + "/info";
// todo 这里待替换
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

改成英文 comment。而且现有的 comment 不清楚其含义:不知道要替换啥

* fix by review comments

Co-authored-by: wangshaoyi <[email protected]>
@@ -34,7 +34,8 @@ class PikaServer;
/* Port shift */
const int kPortShiftRSync = 1000;
const int kPortShiftReplServer = 2000;

//TODO: Temporarily used for rsync server port shift
const int kPortShiftRsync2 = 10001;
const std::string kPikaPidFile = "pika.pid";
const std::string kPikaSecretFile = "rsync.secret";
const std::string kDefaultRsyncAuth = "default";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the provided code patch, here is a brief code review:

  1. The addition of a new constant kPortShiftRsync2 and the accompanying comment suggest that it is temporarily used for rsync server port shifting. Ensure that this temporary usage is properly communicated and documented to prevent confusion.

  2. Overall, the code change seems to be straightforward and doesn't introduce any obvious bug risks.

Improvement suggestions:

  1. Consider using more descriptive names for constants, such as kPortShiftRsyncTemporary, instead of kPortShiftRsync2. This will make the code more readable and maintainable.

  2. Ensure that the use of this temporary variable kPortShiftRsync2 is properly removed or updated once its temporary usage is complete, to avoid leaving unused or misleading code in the system.

  3. It's always good practice to include a comment explaining the reason behind certain code changes, especially when introducing temporary variables/constants or making non-obvious modifications that deviate from the existing codebase.

Remember to thoroughly test the code after making these changes to ensure proper functionality.

@@ -34,9 +34,8 @@ class PikaServer;
/* Port shift */
const int kPortShiftRSync = 1000;
const int kPortShiftReplServer = 2000;
// todo 待移除,使用 kPortShiftRSync
//TODO: Temporarily used for rsync server port shift
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TODO: Temporarily used for rsync server port shift. will be deleted.

* fix by review comments
---------

Co-authored-by: wangshaoyi <[email protected]>
@@ -34,7 +34,8 @@ class PikaServer;
/* Port shift */
const int kPortShiftRSync = 1000;
const int kPortShiftReplServer = 2000;

//TODO: Temporarily used for rsync server port shift. will be deleted.
const int kPortShiftRsync2 = 10001;
const std::string kPikaPidFile = "pika.pid";
const std::string kPikaSecretFile = "rsync.secret";
const std::string kDefaultRsyncAuth = "default";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the code patch you provided, here are some observations:

  1. The addition of kPortShiftRsync2 suggests a new port shift value for the rsync server. However, the comment mentions that it is temporary and will be deleted. Make sure to update or remove this code appropriately once it is no longer needed.

  2. The variable name kPortShiftRsync2 could be improved for better clarity. Consider using a more descriptive name based on its purpose.

  3. Ensure that there are no conflicts or issues arising from the new port shift values (kPortShiftRSync, kPortShiftReplServer, and kPortShiftRsync2) being used in the code elsewhere.

  4. Review the usage of other constants (kPikaPidFile, kPikaSecretFile, and kDefaultRsyncAuth) to ensure they are accurate and consistent throughout the codebase.

  5. It's important to review the surrounding code affected by the changes to confirm that the modifications align with the intended functionality.

Overall, without further context or the complete code, it is difficult to identify any potential bugs or suggest additional improvements beyond what has been mentioned. A comprehensive code review requires examining the broader codebase and understanding the overall architecture and requirements.

@AlexStocks AlexStocks merged commit 5c3e603 into OpenAtomFoundation:unstable Jul 26, 2023
9 checks passed
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
…ess (OpenAtomFoundation#1805)

* define rsync related header file and proto

* feat:add throttle (OpenAtomFoundation#167)

* add_throttle

* feat: implement rsync network tansform (OpenAtomFoundation#169)

implement rsync network transfer

Co-authored-by: wangshaoyi <[email protected]>

* fix action (OpenAtomFoundation#171)

* fix action

* fix

* feat: add load local meta file (OpenAtomFoundation#175)

* add load meta file

* [feat] add rsync client/server code (OpenAtomFoundation#177)

* implement rsync network transfer

* add rsyncclient to syncslaveslot (OpenAtomFoundation#182)

add rsyncclient to syncslaveslot

* feat: add read meta file and data (OpenAtomFoundation#179)

* add read meta file and data

* fix compile error (OpenAtomFoundation#183)

* fix bug

* add rsyncclient to syncslaveslot

* fix compile error

* fix compile error

---------

Co-authored-by: wangshaoyi <[email protected]>

* fix compile error (OpenAtomFoundation#184)

* fix bug

* optimize: add_throttle (OpenAtomFoundation#189)

optimize throttle

* rsyncclient periodically flush meta table (OpenAtomFoundation#192)

rsyncclient periodically flush meta table

* change rsync response (OpenAtomFoundation#190)

* change rsync response

* add debug log for test

* fix rsync client/server bugs

* fix bugs

* add debug log for test

* fix bugs

* fix bugs

* fix bugs

* rix rsync bugs (OpenAtomFoundation#194)

* fix pika rsync bug

* fix bugs

* fix bugs

* fix bugs 1

* fix bugs

* fix rsync bugs (OpenAtomFoundation#195)

* add debug log for test

* fix rsync client/server bugs

* fix bugs

* add debug log for test

* fix bugs

* fix bugs

* fix bugs

* fix bugs

* fix bugs

* fix bugs 1

* fix bugs

---------

Co-authored-by: wangshaoyi <[email protected]>

* remove unused code

* remove unused code

* remove unused code

* remove unused code

* add copyright

* fix by review comments (OpenAtomFoundation#213)

Co-authored-by: wangshaoyi <[email protected]>

* fix by review comments (OpenAtomFoundation#214)

* fix by review comments
Co-authored-by: wangshaoyi <[email protected]>

* fix by review comments (OpenAtomFoundation#216)

* fix by review comments
---------

Co-authored-by: wangshaoyi <[email protected]>

* Optimize rsync wangsy (OpenAtomFoundation#217)

* fix by review comments

Co-authored-by: wangshaoyi <[email protected]>

* fix by review comments (OpenAtomFoundation#218)

* fix by review comments
---------

Co-authored-by: wangshaoyi <[email protected]>

---------

Co-authored-by: wangshaoyi <[email protected]>
Co-authored-by: Yuecai Liu <[email protected]>
Co-authored-by: chejinge <[email protected]>
Co-authored-by: luky116 <[email protected]>
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
…ess (OpenAtomFoundation#1805)

* define rsync related header file and proto

* feat:add throttle (OpenAtomFoundation#167)

* add_throttle

* feat: implement rsync network tansform (OpenAtomFoundation#169)

implement rsync network transfer

Co-authored-by: wangshaoyi <[email protected]>

* fix action (OpenAtomFoundation#171)

* fix action

* fix

* feat: add load local meta file (OpenAtomFoundation#175)

* add load meta file

* [feat] add rsync client/server code (OpenAtomFoundation#177)

* implement rsync network transfer

* add rsyncclient to syncslaveslot (OpenAtomFoundation#182)

add rsyncclient to syncslaveslot

* feat: add read meta file and data (OpenAtomFoundation#179)

* add read meta file and data

* fix compile error (OpenAtomFoundation#183)

* fix bug

* add rsyncclient to syncslaveslot

* fix compile error

* fix compile error

---------

Co-authored-by: wangshaoyi <[email protected]>

* fix compile error (OpenAtomFoundation#184)

* fix bug

* optimize: add_throttle (OpenAtomFoundation#189)

optimize throttle

* rsyncclient periodically flush meta table (OpenAtomFoundation#192)

rsyncclient periodically flush meta table

* change rsync response (OpenAtomFoundation#190)

* change rsync response

* add debug log for test

* fix rsync client/server bugs

* fix bugs

* add debug log for test

* fix bugs

* fix bugs

* fix bugs

* rix rsync bugs (OpenAtomFoundation#194)

* fix pika rsync bug

* fix bugs

* fix bugs

* fix bugs 1

* fix bugs

* fix rsync bugs (OpenAtomFoundation#195)

* add debug log for test

* fix rsync client/server bugs

* fix bugs

* add debug log for test

* fix bugs

* fix bugs

* fix bugs

* fix bugs

* fix bugs

* fix bugs 1

* fix bugs

---------

Co-authored-by: wangshaoyi <[email protected]>

* remove unused code

* remove unused code

* remove unused code

* remove unused code

* add copyright

* fix by review comments (OpenAtomFoundation#213)

Co-authored-by: wangshaoyi <[email protected]>

* fix by review comments (OpenAtomFoundation#214)

* fix by review comments
Co-authored-by: wangshaoyi <[email protected]>

* fix by review comments (OpenAtomFoundation#216)

* fix by review comments
---------

Co-authored-by: wangshaoyi <[email protected]>

* Optimize rsync wangsy (OpenAtomFoundation#217)

* fix by review comments

Co-authored-by: wangshaoyi <[email protected]>

* fix by review comments (OpenAtomFoundation#218)

* fix by review comments
---------

Co-authored-by: wangshaoyi <[email protected]>

---------

Co-authored-by: wangshaoyi <[email protected]>
Co-authored-by: Yuecai Liu <[email protected]>
Co-authored-by: chejinge <[email protected]>
Co-authored-by: luky116 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants