-
Notifications
You must be signed in to change notification settings - Fork 377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow not storing ephemeral node hashes #2568
Conversation
Checking whether it's legit. |
This didn't work before compact ranges and the problems only showed up at particular tree sizes. Need to be sure it's well tested. |
Also when the issue occurred it wasn't always using the latest version, among other changes. |
6f43e42
to
48e1a2a
Compare
It's covered by tests already (and they pass). I factored out #2579 which only removes the reading of ephemeral nodes, and the tests pass. Since not reading them is ok, there is no point in writing them, hence this PR. |
48e1a2a
to
2c6a5db
Compare
2c6a5db
to
821aac4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering about release strategy for this set of changes - perhaps this is large enough to merit a bump to 1.4
, and maybe there should even be a 1.4.0
with the "don't read ephemeral nodes" change and then a 1.4.1
with this "don't write ephemeral nodes" included so that folks can be happy that they're good on 1.4.0
before trying 1.4.1
, because there'll be no roll-back past the "don't read ephemeral nodes" version in case of trouble?
Yes, there can be special effects depending on how this is rolled out (specifically, a new job writes in new format, and an old job tries reading it the old way while being rolled out). I'll leave this unmerged for now and see how far I can get without it. The 2 separate releases SGTM. |
821aac4
to
d8e04dd
Compare
I made more research on the effect of this change, and it looks like submitting this PR is backwards compatible with versions after #305 [Jan 2017, Upd: There is a later commit, see below. |
05085c4
to
720143b
Compare
If this update is rolled out after df47465 (or If this update is rolled out with skipping df47465 (or
The amount of time in which this can happen is limited by:
If this effect is completely undesirable (and there is no way to rollout
|
They are not used for reads, everything is based on compact ranges which don't use the ephemeral nodes.
0d757ca
to
f3fa5a8
Compare
f3fa5a8
to
f9e954f
Compare
f9e954f
to
bdec86c
Compare
@AlCutter @mhutchinson I ended up with a safer approach. This PR now introduces a flag that can enable the new behaviour only for certain trees. We can roll this out for a few trees first, and, once made sure it works, fully enable it. So, I propose to cut a release WDYT? |
bdec86c
to
8b542eb
Compare
This change adds
--tree_ids_with_no_ephemeral_nodes
flag to the sequencer,which allows disabling the writes of ephemeral nodes for a subset of Trillian
trees. The plan is to disable it unconditionally in the next release, but the
flag helps to test the new behaviour safely on a subset of trees.
The stored ephemeral nodes are not used because all the higher level logic uses
compact ranges, operating only with the "perfect" nodes. Since commit df47465 on
Jul 13, 2021 ephemeral nodes are never read from the tree storage.
Not storing ephemeral nodes also has a positive performance effect. Previously,
every sequencing transaction for a typical tree of hundreds of millions entries
would write at least 4 tiles at the tree edge, because it "touched" all the
ephemeral nodes. Now, it will only touch perfect nodes, which for a typical
transaction are in the bottom 1-2 tiles.
Relatedly, now that only non-ephemeral node updates trigger a tile write, every
tile can be written at most 256 times, whereas previously tiles could be written
millions of times (e.g. the root tile of the tree). It led to issues like #1188
which is the result of needing to scan / query millions of rows.
Checklist