Tx tables fix #3888
Tx tables fix #3888
Conversation
There was a problem hiding this comment.
There are a lot of places where we have to check the value of useTxTables_. It's seems like this might be a little error prone. What do you think of creating two separate classes, one for when useTxTables_ is true, and one for when it's false? The class used in the false case would just return an empty optional for all of the functions that query the tx tables.
| @@ -277,7 +277,11 @@ getTxHistory( | |||
| * @param j Journal. | |||
| * @return Vector of pairs of found transactions and their metadata | |||
| * sorted in ascending order by account sequence. | |||
| * Also number of transactions processed. | |||
| * Also number of transactions processed or skpiied. | |||
| return true; | ||
| }); | ||
| /* else use shard databases, if available */ | ||
| if (shardStoreExists() && useTxTables_) |
There was a problem hiding this comment.
I think all of these functions can just return early if useTxTables_ is false. There's no need to do anything else. So at the top of the function:
if(!useTxTables_)
return {};
| * Also number of transactions processed or skpiied. | ||
| * If this number is >= 0, then it means number of transactions | ||
| * processed, if it is < 0, then ~number means number of transactions | ||
| * skipped. We need to skip some quantity of transactions if option |
There was a problem hiding this comment.
Returning ~number for the number of transactions skipped is a very strange interface. I'd suggest returning -number instead - unless there's some strange reason why ~number is better, in which case we should document that reason (here and other functions as well).
There was a problem hiding this comment.
I'll change the interface. My guess is that he went with ~number to avoid the awkward case of effectively saying something like total = -total where total == 0. But that's not really a big deal; the 0 case still works with the change.
| return !res; | ||
| }); | ||
| return res; | ||
| /* else use shard databases, if available */ |
There was a problem hiding this comment.
The code is better off without comments that repeat what the next line of code does (here and in many other places in this file). You don't need to change that now (it's a minor nit), but new code should not include such comments.
There was a problem hiding this comment.
Noted. I'll consider removing these as I dig deeper into refactoring.
There was a problem hiding this comment.
Ended up removing them when addressing CJ's comment about returning early when transaction tables are disabled. Done
| * @return True if node ledger DB exists. | ||
| */ | ||
| bool | ||
| existsLedger() | ||
| { | ||
| return !!lgrdb_; | ||
| return (bool)lgrdb_; |
There was a problem hiding this comment.
the bool() operator automatically evaluates whether a unique_ptr contains something, so casting is redundant.
Also, the way to cast this if you really needed to would be by using something like static_cast(anotherpodtype);
There was a problem hiding this comment.
The compiler actually doesn't invoke the bool operator without the cast. Changed to static_cast instead.
| * @return True if node transaction DB exists. | ||
| */ | ||
| bool | ||
| existsTransaction() | ||
| { | ||
| return !!txdb_; | ||
| return (bool)txdb_; |
There was a problem hiding this comment.
See casting comment above.
There was a problem hiding this comment.
Changed to static_cast
I like that approach in certain scenarios. For instance I used it with the |
undertome
added
the
Passed
Ran the test again - it checks out. |
This is just an extension for a previously approved PR - #3815 which is from a deleted repository.
I added some changes in an attempt to improve logic and readbility. Fortunately these changes are included in a separate commit to make things easier.