not to save unclean object database solution for issue #946 #1311

cogutvalera · 2018-09-08T17:07:13Z

PR for #946 issue "Possible to save unclean object database to file during replay"

abitmore · 2018-09-08T23:46:50Z

programs/witness_node/main.cpp

@@ -155,7 +155,7 @@ int main(int argc, char** argv) {
   if (unhandled_exception)
   {
      elog("Exiting with error:\n${e}", ("e", unhandled_exception->to_detail_string()));
-      node->shutdown();
+      node->shutdown(false);


I think this is not correct.

Please carefully evaluate all scenarios and the need of passing in different parameters. Best leave comments in code explaining why.

sorry forgot here to check "replay-blockchain" mode for condition execution with different params

abitmore · 2018-09-08T23:50:33Z

libraries/chain/db_management.cpp

@@ -220,7 +220,8 @@ void database::close(bool rewind)
   // DB state (issue #336).
   clear_pending();

-   object_database::flush();
+   if (is_clean)
+      object_database::flush();


I think it's better to change the parameter name to do_flush or similar.

agree with you do_flush name is better and makes more sense

abitmore · 2018-09-08T23:52:02Z

libraries/chain/include/graphene/chain/database.hpp

@@ -109,7 +109,7 @@ namespace graphene { namespace chain {
          * Will close the database before wiping. Database will be closed when this function returns.
          */
         void wipe(const fc::path& data_dir, bool include_blocks);
-         void close(bool rewind = true);
+         void close(bool rewind = true, bool is_clean = true);


Please add in-code documentation here.

abitmore · 2018-09-08T23:54:57Z

libraries/app/include/graphene/app/application.hpp

@@ -53,7 +53,7 @@ namespace graphene { namespace app {
         void initialize(const fc::path& data_dir, const boost::program_options::variables_map&options);
         void initialize_plugins( const boost::program_options::variables_map& options );
         void startup();
-         void shutdown();
+         void shutdown(bool is_clean = true);


Best to have doc here as well.

abitmore · 2018-09-09T07:58:31Z

programs/witness_node/main.cpp

+         the on-disk object database would be considered clean and be loaded into memory directly, that's why we need to
+         pass do_flush = FALSE in event of exception while rebuilding object graph by replaying all blocks.
+      */
+      if( options.count("replay-blockchain") )


Not correct. Replay can be triggered due to several reasons, having replay-blockchain specified in options is only one of them.

Thanks. Researching all reasons.

abitmore · 2018-09-09T07:59:37Z

libraries/chain/include/graphene/chain/database.hpp

@@ -109,7 +109,15 @@ namespace graphene { namespace chain {
          * Will close the database before wiping. Database will be closed when this function returns.
          */
         void wipe(const fc::path& data_dir, bool include_blocks);
-         void close(bool rewind = true, bool is_clean = true);
+         /**
+          * @brief Shutdown application


This function is in database class but not application.

abitmore · 2018-09-09T08:01:05Z

libraries/chain/include/graphene/chain/database.hpp

-         void close(bool rewind = true, bool is_clean = true);
+         /**
+          * @brief Shutdown application
+          * @param rewind pop all of the blocks that we can given our undo history, 


This sentence is a bit hard to understand.

Thanks, ok.

abitmore · 2018-09-09T14:26:36Z

programs/witness_node/main.cpp

@@ -155,6 +157,10 @@ int main(int argc, char** argv) {
   {
      elog("Exiting with error:\n${e}", ("e", unhandled_exception->to_detail_string()));

+      chain::block_database block_id_to_block;
+      block_id_to_block.open(data_dir / "database" / "block_num_to_block");


We should not put logic about underlying files in main(). Please do it in database class. (Note: I haven't checked you code since it shouldn't be here, the logic can be wrong if moved to database)

OK. Thanks !

cogutvalera · 2018-09-09T19:00:55Z

now seems much better and much simpler solution with checking undo_db is disabled or not, what are your thoughts about this solution ? is it correct ? or I've missed something again ?

Thanks !

abitmore · 2018-09-09T19:18:28Z

programs/witness_node/main.cpp

-            last_block->block_num() > node->chain_database()->head_block_num()
-         )
-      )
+      if( node->chain_database()->undo_db_disabled() )
         node->shutdown(false);


I don't think it's good to deal with database in main(). IMHO a better approach would be telling application either "we got an error, please shutdown" or "we are fine, please shutdown", and let application decide what to do. Then, perhaps application doesn't know what to do either and will pass the info to database and let database decide, or perhaps it knows what to do and do it accordingly.

Perhaps I think the best way to put this logic inside application and not inside database, IMHO application must know if there was a exception during replay mode and that database must not be flushed. I think this logic in application makes more sense than if it would be in database.

abitmore · 2018-09-10T12:57:10Z

libraries/app/application.cpp

 {
   if( my->_p2p_network )
      my->_p2p_network->close();
   if( my->_chain_db )
   {
-      my->_chain_db->close(true, do_flush);
+      if (after_exception)
+         my->_chain_db->close(true, my->_chain_db->undo_db_enabled());


This code looks a bit ugly to me.

I haven't checked whether the whole logic is correct yet.

abitmore · 2018-09-10T12:58:39Z

The more I think about the issue and read your code, the more strongly I feel that we should refactor/rewrite some code buried deeply but not try to add code in main() to handle uncaught exceptions. For example, why database::reindex() didn't catch exceptions in the first place and handle them locally then return properly?

@cogutvalera please think/analyze before coding. The way you trying to fix the issue (write some code then change over and over) has led to unnecessary (wasted) work/efforts.

cogutvalera · 2018-09-10T13:05:27Z

I thought we do not need rewrite a lot of code so deeply. IMHO I thought that we need to fix this issue with minor changes. Should we make deep code refactor/changes for this issue and not minor changes ?

abitmore · 2018-09-10T13:14:28Z

Try to avoid unnecessary/duplicate efforts. If we simply patch it this time, we'll have to spend more time/resources to cleanup the patch in the future. This issue is not a high priority one, so we have much time to fix it in a good manner. If we can improve the code structure while fixing this issue, we'll gain much more value.

Don't let the estimated hours bind you down. If you got extra useful work done, I'm sure you'll be compensated. On the opposite, I don't feel good to compensate the hours for writing some code which have to be rewritten later.

cogutvalera · 2018-09-10T13:29:52Z

Ok. Thanks ! Understood ! I won't be scared next time to make more code changes for more ideal solutions even if it will require deep code refactoring ! So I will think/analyze this issue more deeply then with best design paterns, design principles and approaches.

I do not worry about spending more time and about compensation, just tried to make changes as small as possible without refactoring a lot current architecture, but I was wrong because for more ideal architecture we must not be scared about deep code refactoring and architecture refactoring.

Thanks !

…res#946

cogutvalera · 2018-09-24T16:37:01Z

I've created Error Handling new issue #1338 for better architecture design and approach, but this issue is fixed by simple enough solution with minor changes, because new Error Handling implementation by Eithers must be implemented as NEW issue, it requires a lot of code changes and deep architecture design refactor. Of course we can implement it step by step, just need more discussion about Eithers.

cogutvalera · 2018-10-19T10:59:03Z

is this PR and related issue within low priority scope ?

pmconrad · 2018-10-19T12:11:32Z

Argh. Please remove that huge comment, having the discussion here on github is sufficient.
You can update your original message with a short summary of discussion and solution.

cogutvalera · 2018-10-19T12:48:24Z

ok sure ! Thank you very much !

cogutvalera · 2018-10-19T13:13:53Z

Done ! Thank you !

cogutvalera · 2018-11-01T06:57:03Z

is something wrong with this PR or with my solution ? maybe I've missed anything ? Or we can merge and close it maybe ? What are your thoughts friends ?

Thank you !

cogutvalera · 2018-11-22T06:07:09Z

@abitmore @pmconrad @jmjatlanta @oxarbitrage Guys what should we do with this PR ? Look please when you will have time.

Thanks !

pmconrad · 2019-04-11T13:13:21Z

The issue was inadvertently fixed by #1529

abitmore requested changes Sep 8, 2018

View reviewed changes

abitmore reviewed Sep 9, 2018

View reviewed changes

abitmore reviewed Sep 10, 2018

View reviewed changes

abitmore added this to the 201812 - Feature Release milestone Sep 14, 2018

Possible to save unclean object database to file during replay bitsha…

7c22c46

…res#946

cogutvalera force-pushed the issue_946 branch from 6eb5441 to 7c22c46 Compare September 24, 2018 16:29

fixed typo

0a61f4f

Summary comment instead of huge

f5840ef

cogutvalera mentioned this pull request Jan 19, 2019

Possible to save unclean object database to file during replay #946

Closed

8 tasks

oxarbitrage modified the milestones: 201902 - Feature Release, Future Feature Release Jan 29, 2019

pmconrad closed this Apr 11, 2019

pmconrad removed this from the Future Feature Release milestone Apr 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

not to save unclean object database solution for issue #946 #1311

not to save unclean object database solution for issue #946 #1311

cogutvalera commented Sep 8, 2018

abitmore Sep 8, 2018

cogutvalera Sep 9, 2018

abitmore Sep 8, 2018

cogutvalera Sep 9, 2018

abitmore Sep 8, 2018

cogutvalera Sep 9, 2018

abitmore Sep 8, 2018

cogutvalera Sep 9, 2018

abitmore Sep 9, 2018

cogutvalera Sep 9, 2018

abitmore Sep 9, 2018

cogutvalera Sep 9, 2018

abitmore Sep 9, 2018

cogutvalera Sep 9, 2018

abitmore Sep 9, 2018 •

edited

Loading

cogutvalera Sep 9, 2018

cogutvalera commented Sep 9, 2018

abitmore Sep 9, 2018

cogutvalera Sep 9, 2018

cogutvalera Sep 9, 2018

abitmore Sep 10, 2018

abitmore commented Sep 10, 2018

cogutvalera commented Sep 10, 2018 •

edited

Loading

abitmore commented Sep 10, 2018

cogutvalera commented Sep 10, 2018

cogutvalera commented Sep 24, 2018

cogutvalera commented Oct 19, 2018 •

edited

Loading

pmconrad commented Oct 19, 2018

cogutvalera commented Oct 19, 2018

cogutvalera commented Oct 19, 2018

cogutvalera commented Nov 1, 2018 •

edited

Loading

cogutvalera commented Nov 22, 2018

pmconrad commented Apr 11, 2019

not to save unclean object database solution for issue #946 #1311

not to save unclean object database solution for issue #946 #1311

Conversation

cogutvalera commented Sep 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore Sep 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cogutvalera commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore commented Sep 10, 2018

cogutvalera commented Sep 10, 2018 • edited Loading

abitmore commented Sep 10, 2018

cogutvalera commented Sep 10, 2018

cogutvalera commented Sep 24, 2018

cogutvalera commented Oct 19, 2018 • edited Loading

pmconrad commented Oct 19, 2018

cogutvalera commented Oct 19, 2018

cogutvalera commented Oct 19, 2018

cogutvalera commented Nov 1, 2018 • edited Loading

cogutvalera commented Nov 22, 2018

pmconrad commented Apr 11, 2019

abitmore Sep 9, 2018 •

edited

Loading

cogutvalera commented Sep 10, 2018 •

edited

Loading

cogutvalera commented Oct 19, 2018 •

edited

Loading

cogutvalera commented Nov 1, 2018 •

edited

Loading