Kvapi rocksdb nodeos integration first working version check in by linhuang-blockone · Pull Request #9340 · EOSIO/eos

linhuang-blockone · 2020-07-24T18:58:11Z

Change Description

This PR checks in the first KV API RocksDB working version, serving as a baseline example for the team. All new test cases have passed. Comments from Bucky at the review meeting have not yet been incorporated. They are marked by "#warning: TODO" so compiler will generate a warning and we will not forget. Please pay attention to them as they represent new direction. I will incorporate them in my next PR.

Change Type

Select ONE

Documentation

Stability bug fix

[ X] Other

Other - special case

Consensus Changes

Consensus Changes

API Changes

API Changes

Documentation Additions

Documentation Additions

…-scenarios-test.py while chain_kv is reevaluated

… accomadate change of state memory layout

TimothyBanks · 2020-07-24T20:22:33Z

+                  std::vector<char> prefix = rocksdb_contract_kv_prefix;
+                  b1::chain_kv::append_key(prefix, kvdisk_id.to_uint64_t());
+                  it->Seek(b1::chain_kv::to_slice(rocksdb_contract_kv_prefix));
+                  while(it->Valid()) {


It would be nice if the rocksdb::Iterator supported the appropriate iterator concept to allow for range base for loops. This might be something that we could wrap within chain_kv.

I think we should wait on these. @revl needs to make several changes for this.

@revl is working on this

TimothyBanks · 2020-07-24T20:25:04Z

            if ( header.version < kv_object::minimum_snapshot_version )
               return;
+            if (conf.use_rocksdb_for_disk) {
+               rocksdb::WriteBatch batch;


My opinion is that the controller should have no direct dependency on rocksdb types. Can this functionality be pushed down into the chain_kv api?

yes, and @revl will handle this after this PR is committed.

@revl is working on this

TimothyBanks · 2020-07-24T20:35:27Z

+            snapshot->write_section<value_t>([this]( auto& section ){
+               // This ordering depends on the fact the eosio.kvdisk is before eosio.kvram and only eosio.kvdisk can be stored in rocksdb.
+               if (db.get<kv_db_config_object>().using_rocksdb_for_disk) {
+                  std::unique_ptr<rocksdb::Iterator> it{kv_database.rdb->NewIterator(rocksdb::ReadOptions())};


Could this functionality be pushed down into chain_kv?

@linhuang-blockone apply my comment about leaving refactoring for @revl for all these comments about pushing this down into chain_kv. @revl and @TimothyBanks can work this out later.

@revl is working on this

TimothyBanks · 2020-07-24T20:37:34Z

 chainbase::database& controller::mutable_db()const { return my->db; }

 const fork_database& controller::fork_db()const { return my->fork_db; }
+b1::chain_kv::database& controller::kv_database() { return my->kv_database; }


Is there any need for a const overload of this method?

Whether or not there is, I think @revl should look at that. Lin doesn't need to do it here since what he has implemented obviously isn't needing it.

(from Lin) Cannot add const here, as db and undo_stack parameters in create_kv_rocksdb_context are non-const.

TimothyBanks · 2020-07-24T20:37:50Z


 const fork_database& controller::fork_db()const { return my->fork_db; }
+b1::chain_kv::database& controller::kv_database() { return my->kv_database; }
+b1::chain_kv::undo_stack& controller::kv_undo_stack() { return my->kv_undo_stack; }


Is there any need for a const overload of this method?

create_kv_rocksdb_context() needs kv_database and kv_undo_stack as argutments but kv_database() and kv_undo_stack() aren't accessible because they are private.

not following why @kimjh2005 thinks this, since the code does that and it compiles

TimothyBanks · 2020-07-27T15:17:44Z

+            auto kv = kv_it.get_kv();
+
+            // kv is always non-null due to the check of is_valid()
+            *found_key_size = kv->key.size();


Who is responsible for the null check on found_key_size and found_value_size?

TimothyBanks · 2020-07-27T15:18:54Z

+      kv_resource_manager                      resource_manager;
+      const kv_database_config&                limits;
+      uint32_t                                 num_iterators = 0;
+      std::shared_ptr<const std::vector<char>> temp_data_buffer;


Who is this shared with?

TimothyBanks · 2020-07-27T15:19:51Z

+   }; // kv_iterator_rocksdb
+
+   struct kv_context_rocksdb : kv_context {
+      b1::chain_kv::database&                  database;


I'm just wanting to make sure that there are guarantees that make sure the database (and undo stack below) stay in scope with the kv_context.

kv_context_rocksdb is created and destroyed by apply_context, which is created and destroyed by controller. Controller creates the database on startup and destroys it on shutdown.

TimothyBanks · 2020-07-27T15:21:01Z

+            }
+            FC_LOG_AND_RETHROW()
+         }
+         CATCH_AND_EXIT_DB_FAILURE()


Are we sure we want to terminate the program?

If this isn't a refactoring from the existing code, then we definitely cannot

TimothyBanks · 2020-07-27T15:22:41Z

+            }
+            FC_LOG_AND_RETHROW()
+         }
+         CATCH_AND_EXIT_DB_FAILURE()


One last time, do these warrant program termination?

brianjohnson5972 · 2020-07-27T16:56:45Z

 #include <eosio/chain/types.hpp>

 namespace eosio { namespace chain {
+   class kv_db_config_object : public chainbase::object<kv_db_config_object_type, kv_db_config_object> {


Since KV and DB storage will always be the same (either both in RocksDB or both in chainbase), this object should be named something like backing_storage_config_object and should be defined in an *_objects file that isn't specific to KV. (In general we should avoid using "db" in anything refering to RocksDB vs chainbase, since we use kv and db to distinguish the intrinsics)

brianjohnson5972 · 2020-07-27T16:58:05Z

+      OBJECT_CTOR(kv_db_config_object)
+
+      id_type id;
+      bool    using_rocksdb_for_disk = false;


need to drop the "_for_disk" extension here, on the similarly named functions, and elsewhere

I think it would be good to make this an enum that is called "backing_storage" with the default value being chain_base, and could be set to rocks_db. This way if we add other backing stores in the future, we could just add them here, instead of having a new parameter and having to validate that parameter's value doesn't conflict with this parameter's value.

brianjohnson5972 · 2020-07-27T18:35:24Z

+      if (db.get<kv_db_config_object>().using_rocksdb_for_disk)
+         return;
+      auto& idx = db.get_index<kv_index, by_kv_key>();
+      auto  it  = idx.lower_bound(boost::make_tuple(kvdisk_id, name{}, std::string_view{}));


These should all be kvram_id and eosio.kvram

brianjohnson5972 · 2020-07-27T18:37:36Z

                                   composite_key_compare<std::less<name>, std::less<name>, unsigned_blob_less>>>>;

+   inline void use_rocksdb_for_disk(chainbase::database& db) {
+      if (db.get<kv_db_config_object>().using_rocksdb_for_disk)


store off db.get<kv_db_config_object>() in a variable and use the variable here and below

brianjohnson5972 · 2020-07-27T20:57:11Z

         kv_iterators.resize(1);
         kv_destroyed_iterators.clear();
         if (!context_free) {
+#warning TODO: Remove kv_ram. Rename kv_disk


fix this comment and switch code below to be for kv_ram instead of kv_disk

brianjohnson5972 · 2020-07-28T19:03:32Z

         }

+         // skip the kv_db_config as it only determines where the kv-database is stored
+         if (std::is_same_v<value_t, kv_db_config_object>) {


Remove this. If you don't add it to the using definition (above) for controller_index_set, then it won't be present in the walk_indices.

brianjohnson5972 · 2020-07-28T19:10:07Z

         if constexpr (std::is_same_v<value_t, kv_object>) {
            if ( header.version < kv_object::minimum_snapshot_version )
               return;
+            if (conf.use_rocksdb_for_disk) {


Same issue that I mentioned in writing snapshot section above, if you have a kv_object then we are using chainbase as the backing store and use_rocksdb won't be set. The check should be used after reading the data, before determining if you should write to the rocksdb (key value) backing store or the chainbase backing store. That code should be through an abstraction, that takes the snapshot data and then there will be a chainbase implementation or a rocksdb (key value) implementation (not sure if this would go in kv_context_*** or not, it probably can)

brianjohnson5972 · 2020-07-28T21:48:53Z

+      // Currently kv_undo_stack returns a wrong revision after restart,
+      // failing tests/terminate-scenarios-test.py.
+      // As chain_kv is being rewritten, comment out this check for now
+      // and retest after chain_kv rewriting is finished.


Has Timothy been made aware of this problem in kv_undo_stack?

brianjohnson5972 · 2020-07-28T21:53:43Z

 class kv_tester : public tester {
 public:
   kv_tester() {
+   eosio::chain::use_rocksdb_for_disk(const_cast<chainbase::database&>(control->db()));


fix indentation, and why don't you use your helper method you added?

And this line means that kv_tester only tests kv with rockdb and not chainbase. To test both you can use BOOST_AUTO_TEST_CASE_TEMPLATE below and make kv_tester templated on backend_store (chainbase, rocksdb - see other comment about use_rocksdb_for_disk not being a bool) . See snapshot_tests.cpp for examples of how to use.

brianjohnson5972 · 2020-07-28T22:09:28Z

      // BOOST_TEST(iterlimit({{N(eosio.kvram), 0xFFFFFFFF, false}, {N(eosio.kvdisk), 1, false}}) == "Too many iterators");
   }

+   // Make sure that a failed transaction correctly rolls back changes to the database,


now I see what you are doing. Use my suggestion above, revert these methods and the BOOST_AUTO_TEST_CASE_TEMPLATE will take care of testing both chainbase and rocksdb.

or you cans use BOOST_DATA_TEST_CASE_F, but no reason that I can see to repeat all these tests 3 times, unless I am missing something.

…ntegration

brianjohnson5972

Checking this PR in as-is and @linhuang-blockone will be breaking up the cleanup into their own individual PRs.

linhuang-blockone added 3 commits July 22, 2020 11:58

KV API RocksDB integration first checkin

d08e18e

KV API RocksDB integration first checkin

3aab484

add #warnings for not yet incorporated comments from Bucky

4847e90

linhuang-blockone requested review from TimothyBanks, brianjohnson5972, kryton, larryk85, nksanthosh and revl July 24, 2020 18:58

linhuang-blockone added 2 commits July 25, 2020 17:18

Temporarily disable revision check to get around failure of terminate…

2ba0518

…-scenarios-test.py while chain_kv is reevaluated

Update commit number for eosio-resume-from-state in pipeline.jsonc to…

5c43133

… accomadate change of state memory layout

TimothyBanks suggested changes Jul 27, 2020

View reviewed changes

linhuang-blockone added 2 commits July 27, 2020 14:43

Use correct commit hash after state snapshot is collected

6fdda4b

log ret value for debugging

513e0e1

brianjohnson5972 suggested changes Jul 28, 2020

View reviewed changes

linhuang-blockone force-pushed the kvapi_rocksdb_nodeos_integration branch from 31c34f7 to 513e0e1 Compare August 1, 2020 19:44

Merge branch 'rocksdb_nodeos_integration' into kvapi_rocksdb_nodeos_i…

1505dde

…ntegration

brianjohnson5972 reviewed Aug 4, 2020

View reviewed changes

Comment thread libraries/chain/include/eosio/chain/exceptions.hpp

brianjohnson5972 approved these changes Aug 4, 2020

View reviewed changes

revl approved these changes Aug 4, 2020

View reviewed changes

brianjohnson5972 merged commit 4111468 into rocksdb_nodeos_integration Aug 4, 2020

larryk85 deleted the kvapi_rocksdb_nodeos_integration branch August 31, 2020 19:49

Conversation

linhuang-blockone commented Jul 24, 2020 • edited by revl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Change Type

Consensus Changes

API Changes

Documentation Additions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

linhuang-blockone commented Jul 24, 2020 •

edited by revl

Loading