re: #14121 (review)
I don’t really like the change in behavior to Commit/Rewind. Commit just does DB/disk persistence, which is why errors can generally be ignored
Up to you but I’d encourage you to take another look. Your comment sounds more like a reaction to the method naming and not the substance of the change. I agree with you naming in my suggested change is not ideal. I actually wanted to call the base Commit()
method UpdatePosition()
and the overridden Commit()
method Flush()
, but I reverted back to your names to try to avoid adding unrelated differences. (IMO, all the mentions of committing and atomicity in this PR are misleading because almost nothing about the implementation is atomic. It is true that updates of m_best_block, DB_BEST_BLOCK, and fltr*.dat files are ordered, but operations being ordered is different from them being atomic, and it feels like too much is implied by the current naming even if the code is technically correct.)
In any case, here are the advantages of my change beyond reducing the amount of code:
- It updates m_best_block_index in the same place as DB_BEST_BLOCK.
- It flattens control flow, getting rid the BlockFilterIndex::Rewind calling BaseIndex::Rewind calling BlockFilterIndex::Commit calling BaseIndex::Commit sequence and the whole call super anti-pattern.
It’s sounds crazy to me to cite updating m_best_block_index and DB_BEST_BLOCK in different places instead of the same place as an advantage of the current code. This seems like a footgun and anti-ergonomic decision, like putting the steering wheel on the outside of a car. When you want to turn your car left, do you just want to turn the wheel? Or is it better to stop the car, get out, point the wheel, make the turn, stop again, straighten, then keep going? When you update the position, do you just want to call one method? Or is it better to manually set a member variable and go through a rube-goldberg sequence of virtual method calls?