Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Saving and restoring undo stack at shutdown and startup #9772

Merged
merged 9 commits into from
Dec 11, 2020

Conversation

linhuang-blockone
Copy link
Contributor

@linhuang-blockone linhuang-blockone commented Dec 10, 2020

Change Description

This PR saves RocksDB backing store undo stack when shutdown and restores undo stack upon startup.

This is a draft PR calling for review and quick turn around of resolving any design/coding issues.

Change Type

Select ONE:

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Testing Changes

Select ANY that apply:

  • New Tests
  • Existing Tests
  • Test Framework
  • CI System
  • Other

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

In <the configured data-dir>/state directory, a new file undo_stack.dat is introduced if --backing-store rocksdb is used . It saves RocksDB undo stack's state before node shutdown and is used to restore nodeos's undo stack at startup.

@@ -252,6 +252,11 @@ struct controller_impl {
{ check_protocol_features( timestamp, cur_features, new_features ); }
);

// Might be too late?
if ( cfg.backing_store == backing_store_type::ROCKSDB ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we don't just do this in the constructor for kv_db? It is passed cfg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change to use constructor of kv_db.

@@ -139,7 +139,7 @@ class session {

/// \brief Constructs a child session from another instance of the same session type.
explicit session(session& parent, std::nullptr_t);
session(const session&) = delete;
//session(const session&) = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why you needed to remove this? I don't think you should need a copy constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Tim also spot this. I have changed it back in the next commit.

libraries/chain_kv/include/b1/session/session.hpp Outdated Show resolved Hide resolved
libraries/chain_kv/include/b1/session/undo_stack.hpp Outdated Show resolved Hide resolved
libraries/chain_kv/include/b1/session/undo_stack.hpp Outdated Show resolved Hide resolved
libraries/chain_kv/include/b1/session/undo_stack.hpp Outdated Show resolved Hide resolved
@TimothyBanks
Copy link
Contributor

Thanks for the help Lin.

@linhuang-blockone
Copy link
Contributor Author

Thanks for the help Lin.

Thank you Tim, Bucky, and Brian for all the help. My last commit made it serialization and deserialization work (I verified via logging). I will incorporate your other comments shortly.

@@ -508,8 +508,14 @@ shared_bytes::shared_bytes_iterator<Iterator_traits>::operator[](size_t index) c
return m_buffer[index];
}

struct shared_bytes_reflect{
Copy link
Contributor

Choose a reason for hiding this comment

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

So, instead of making this struct and reflecting it. What you should do is this then.

Instead of using the FC_REFLECT, because the internals of shared_bytes are not very usable through reflection.

Add two public methods to shared_bytes:

template <typename Stream>
inline Stream& operator<<(Stream& ds, const shared_bytes& b) {
   ds << b.size();
   ds.write(b.data(), b.size());
   return ds;
}

and

template <typename Stream>
inline Stream& operator>>(Stream& ds, shared_bytes& b) {
   std::size_t sz;
   ds >> sz;
   shared_bytes tmp = {sz};
   ds.read(tmp.data(), tmp.size());
   b = tmp;
   return ds;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

shared_bytes already has an out stream overload and a method that can create a shared_bytes from a hex string (which is what is streamed out). You would just need to provide the operator for streaming in.


size_t num_updated_keys; fc::raw::unpack( ds, num_updated_keys );
for( size_t j = 0; j < num_updated_keys; ++j ) {
shared_bytes_reflect raw_key; fc::raw::unpack( ds, raw_key );
Copy link
Contributor

Choose a reason for hiding this comment

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

If you implement the above methods for shared_bytes, this becomes...

shared_bytes key;
ds >> key;

shared_bytes value;
ds >> value;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! This simplifies the code. Changed.

auto value = session.read(key);

if ( value ) {
shared_bytes_reflect raw_key;
Copy link
Contributor

Choose a reason for hiding this comment

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

with the datastream operators this would become...

out << key;
out << value;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@linhuang-blockone linhuang-blockone merged commit 972bafe into develop Dec 11, 2020
@larryk85 larryk85 deleted the undo_stack_initialization branch December 11, 2020 16:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants