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

fix solo repldev will be nullptr in map when homestore restart #199

Merged

Conversation

zichanglai
Copy link
Contributor

@zichanglai zichanglai commented Oct 7, 2023

This PR fix issue when HS restarts, the Solo ReplDev in map will be set to nullptr. it is caused by an unexpected std::move() operation in src/lib/replication/service/repl_service_impl.cpp Line80

@zichanglai zichanglai requested a review from hkadayam October 7, 2023 08:08
@zichanglai zichanglai force-pushed the fix-solo-dev-nullptr-when-restart branch from 53a2285 to c02c730 Compare October 7, 2023 09:52
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (27e7601) 69.11% compared to head (e5ffc33) 69.19%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage   69.11%   69.19%   +0.08%     
==========================================
  Files          93       93              
  Lines        7556     7558       +2     
  Branches      965      967       +2     
==========================================
+ Hits         5222     5230       +8     
- Misses       1868     1869       +1     
+ Partials      466      459       -7     
Files Coverage Δ
src/lib/replication/service/repl_service_impl.cpp 59.75% <0.00%> (-0.74%) ⬇️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -74,7 +73,7 @@ ReplicationServiceImpl::open_repl_dev(uuid_t group_id, std::unique_ptr< ReplDevL
auto it = m_rd_map.find(group_id);
if (it != m_rd_map.end()) {
// We already loaded the ReplDev, just call the group_id and attach the listener
auto& repl_dev = it->second;
auto repl_dev = it->second;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not clear how is this fixes the problem. At this time, someone already opened the repl_dev and group_id -> repl_dev has been mapped. So how is this storing by referencing or incrementing the ref_count of repl_dev solves this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but at Line80, this repl_dev will be moved, that is why I changed to make a copy of shared_ptr

@zichanglai zichanglai force-pushed the fix-solo-dev-nullptr-when-restart branch from c02c730 to d172f35 Compare October 8, 2023 01:38
@zichanglai zichanglai force-pushed the fix-solo-dev-nullptr-when-restart branch from d172f35 to e5ffc33 Compare October 9, 2023 05:29
Copy link
Collaborator

@xiaoxichen xiaoxichen left a comment

Choose a reason for hiding this comment

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

Approving this as it is clear,
repl_dev was by-reference referencing it->second , later in L80 repl_dev has been std::move() which will set to repl_dev to null and as it is by-reference, it->second be set to null

@zichanglai zichanglai merged commit 169d97e into eBay:master Oct 9, 2023
16 checks passed
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