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 restartability bug for upper layer #333

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

JacksonYao287
Copy link
Contributor

if it is not first-time-boot, the uuid of HO is saved in metaservice. so we need to get the uuid after metaservice is started, otherwise we will get a new default uuid

@JacksonYao287 JacksonYao287 added the bug Something isn't working label Feb 22, 2024
@JacksonYao287 JacksonYao287 added this to the MileStone4.1 milestone Feb 22, 2024
@JacksonYao287 JacksonYao287 self-assigned this Feb 22, 2024
@xiaoxichen
Copy link
Collaborator

This suggests we are wrong in https://github.com/eBay/HomeStore/pull/284/files
for restart, the _our_id in HO is set by homestore_metablk_callback, which will be called after meta svc started in https://github.com/eBay/HomeStore/blob/master/src/lib/homestore.cpp#L210

In the #284 we move the initialization of GenericReplService from start to do_start.

I think we either
a. move https://github.com/eBay/HomeStore/blob/master/src/lib/homestore.cpp#L198 down below L210
or
b. move L198 back to start() , remove wrong comments and take the change in this PR to get uuid in GenericReplService::start() in L217

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.

check back with #284

@JacksonYao287
Copy link
Contributor Author

@xiaoxichen
for a, we can not make this change, since the register_handler happens in the constructor of RaftReplService

meta_service().register_handler(

if we put L198 down below L210, then the handler can not be registered before metaservice#start, which will lead to a missing raft config.

well, b is acceptable, will do this!

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 47.70%. Comparing base (2f5f94d) to head (4d5bebf).

Files Patch % Lines
src/lib/homestore.cpp 0.00% 1 Missing ⚠️
src/lib/replication/service/generic_repl_svc.cpp 0.00% 1 Missing ⚠️
src/lib/replication/service/raft_repl_service.cpp 0.00% 1 Missing ⚠️

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   47.70%   47.70%           
=======================================
  Files         107      107           
  Lines        9305     9307    +2     
  Branches     1209     1209           
=======================================
+ Hits         4439     4440    +1     
+ Misses       4393     4392    -1     
- Partials      473      475    +2     

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

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.

lgtm

Though we had seen an initialized uuid default value for m_repl_app->get_my_repl_id() and do not know the reason, but maybe
RELEASE_ASSERT(!m_my_uuid.value().is_nil(), "Uninitialized RaftReplService UUID");

@JacksonYao287
Copy link
Contributor Author

lgtm

Though we had seen an initialized uuid default value for m_repl_app->get_my_repl_id() and do not know the reason, but maybe RELEASE_ASSERT(!m_my_uuid.value().is_nil(), "Uninitialized RaftReplService UUID");

ok, will add it in the later coming PR

@JacksonYao287 JacksonYao287 merged commit 2301a6d into eBay:master Feb 23, 2024
20 checks passed
@JacksonYao287 JacksonYao287 deleted the fix-restartability branch February 23, 2024 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants