GH-552: max_block_count defaults to 100_000 #558
Conversation
masqrauder
requested review from
czarte,
dnwiebe,
utkarshg6,
kauri-hero and
bertllll
masqrauder
force-pushed
the
GH-552
branch
3 times, most recently
from
f17c2fb to
9565ae3
Compare
|
Thanks @masqrauder I see this has a database migration now - is that meaning its a breaking change for older versions of Node <v0.8.2 essentially? |
There was a problem hiding this comment.
Everything seems good, but I'm unfamiliar with the database migration side of things with that being added.
Approved conditional to another team member reviewing who has written a db migration or reviewed one before @utkarshg6 @bertllll @dnwiebe
|
Not a breaking change. But the way the persistence configuration is designed it requires a migration to set the default since it is currently undefined by default. |
|
fixed #552 |
|
@utkarshg6 we would like you to do a review on this card, as part of expanding our peer review capacity in the team. This is a good one for practice! |
masq_lib/src/constants.rs
Outdated
| @@ -215,3 +215,5 @@ mod tests { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| pub const DEFAULT_MAX_BLOCK_COUNT: u64 = 100_000u64; | |||
There was a problem hiding this comment.
It's maybe unnecessary that you use the numeric type in the literal. Because constants require the type to be mentioned right behind its name in the declaration. 🤷♂️
I'm saying that just because it's so unusual in our team using this format of numbers. Only if forced.
| "DbMigrator: Database successfully migrated from version 7 to 8", | ||
| "DbMigrator: Database successfully migrated from version 8 to 9", | ||
| "DbMigrator: Database successfully migrated from version 9 to 10", | ||
| ]); |
There was a problem hiding this comment.
In general, I'm all okay with this test. But on the other hand, it also isn't exactly as I would expect it to be. I noticed you got inspired from your own migration "8 to 9", and the truth is there it was where you've begun writing in a different style than in which the other migrations are done.
I understand these are almost cosmetics but still, maybe you could have some time to do these simple changes.
- Mostly for formality, hmm, still reasonable, we tended to arrange it so that there is the act properly isolated. The issue in your case is that you make it look as if you were testing all the steps from 0 to 10. It shouldn't be necessary, you agree with me.
So again, for it's better structured like that, we first ran all the migration but yours, where we stopped, and left the database in the state as it would be if you were truly in need of this last migration. Let's say from 0 to 9.
Then, the only thing left for the act is to run the function initialize_to_version where you specify the target as "10".
This way the test is clear about what it is testing.
- Also based on the insight I just threw in, the logs you assert on excessively, are somewhat too much for me, seen as so many records, but together of not much value, bland, only stealing somebody's attention.
The process of gradual migrations is heavily tested on its own, separately. Here I could argue you're not testing the code added but rather the machinery around, which doesn't sound right.
Therefore I'd recommend you not to assert on logs for these database migrations unless it is an actual new log straight from within your implementation of that new migration. Which this is not the case. My opinion is the test would be better free of this massive log assertion.
I want to ask you to implement my ideas here (9 to 10) as well as for the previous migration (8 to 9). Thank you 🙏
Most of the feedback from the RPC providers has indicated that not having a smaller max block count will be a regular timeout issue as the range of scans increases. This will occur if the user is offline for more that a few days.
Implement max_block_count value of 100,000 as default value.
If the RPC response provides another value, then the logic will handle adjusting for that anyways.