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

Catch missing node error when rotating database #4182

Closed
wants to merge 1 commit into from

Conversation

seelabs
Copy link
Collaborator

@seelabs seelabs commented May 27, 2022

While there should never be a missing node when copying the SHAMap,
rippled should not terminate when there's an error rotating the
database. This patch aborts the database rotation rather than aborting rippled.

  • [ x] Bug fix (non-breaking change which fixes an issue)

Copy link
Collaborator

@mtrippled mtrippled left a comment

Choose a reason for hiding this comment

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

LGTM 👍 but fix the spelling of "rotate"

catch (SHAMapMissingNode const& e)
{
JLOG(journal_.error())
<< "Missing node while copying ledger before roate: "
Copy link
Collaborator

Choose a reason for hiding this comment

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

spell check "rotate"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@@ -30,6 +30,7 @@
#include <ripple/nodestore/Scheduler.h>

#include <boost/algorithm/string/predicate.hpp>
#include "ripple/shamap/SHAMapMissingNode.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you use chevrons to enclose this #include? And move it up with the ripple includes please? Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@scottschurr Fixed - also an explanation: I changed my editor configuration and it started "helpfully" adding include files for me. I need to figure out how to turn that off. I missed that is added that for me. Thanks for spotting it!

Choose a reason for hiding this comment

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

你们和雷达币有关吗

Copy link
Contributor

@nbougalis nbougalis Jun 7, 2022

Choose a reason for hiding this comment

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

@haihai423: Short answer: no. Longer answer: Still no. I believe that Radar Coin simply cloned this code (along with several libraries and even entire websites), only changing the name.

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

Changes look good to me once the new #include is fixed up. Thanks.

I'm going to let this code burn in for a while on the same machine that saw the crash yesterday. There's no guarantee that I'll see the same problem, but I may as well give it a try. If I don't see any crashes after it burns in for a few hours I'll give it the thumbs up.

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

I did a burn in with this code over the weekend. Unfortunately, and to no one's surprise, the burning in code neither crashed nor left a message in the log to verify that we really fixed things up with this change. But I think there's a high likelihood that this is the right change.

Let's do this. Thanks for the fix. 👍

While there should never be a missing node when copying the SHAMap,
rippled should not terminate when there's an error rotating the
database. This patch aborts the database rotation rather than aborting rippled.
@seelabs
Copy link
Collaborator Author

seelabs commented Jun 22, 2022

Squashed and marked passed

@seelabs seelabs added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 22, 2022
@nbougalis nbougalis mentioned this pull request Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants