feat: proper move ins/outs for subqueries without 409s #3427
Conversation
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
icehaunter
force-pushed
the
ilia/pr/rmysvxopsvsz
branch
from
744a810 to
88d4e40
Compare
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3427 +/- ##
==========================================
- Coverage 75.20% 75.17% -0.04%
==========================================
Files 51 51
Lines 2743 2743
Branches 405 408 +3
==========================================
- Hits 2063 2062 -1
- Misses 678 679 +1
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
icehaunter
force-pushed
the
ilia/pr/rmysvxopsvsz
branch
from
1229d51 to
90c78c9
Compare
| def increment(%LogOffset{op_offset: :infinity} = log_offset, increment) when increment > 0 do | ||
| %{log_offset | tx_offset: 1, op_offset: increment - 1} | ||
| end |
There was a problem hiding this comment.
This reads as ♾️ + 1 = 0, but I'm guessing this is no longer an increment function and something more akin to a next function so perhaps it should be named as such (perhaps keeping and calling the increment for the non infinity case). Also tx_offset gets set to 1 regardless of what it was before, presumably because the assumption is infinity is only on tx_offset 0, so perhaps that should be asserted. Or perhaps this isn't a general function of LogOffset at all and should just be kept in the module where it's used.
There was a problem hiding this comment.
I personally don't mind this too much, but would like to see a doctest for this case in particular
the ♾️ is really just a stand-in for the "supremum" of a given tx_offset, not an actual infinity, so adding one makes sense in that it moves to the next tx_offset. I see the point in calling this next but probably worth doing separately in a single PR.
That being said, I think I would prefer if this was also overspecified to the last_before_real_offsets which has tx_offset of 0 rather than any tx_offset - the only times we use infinity is for the supremum of the virtual offsets (which can move to the real offsets), and the last of the real offsets, which should not move on to the next tx offset.
There was a problem hiding this comment.
supremium or infinity, they both have the same issue, it's not a real increment. i still think next makes more sense but happy for that to be addressed in a separate PR
packages/sync-service/lib/electric/shapes/consumer/move_handling.ex
Outdated
Show resolved
Hide resolved
icehaunter
force-pushed
the
ilia/pr/rmysvxopsvsz
branch
from
3a1f9fb to
ebbacf5
Compare
| def increment(%LogOffset{op_offset: :infinity} = log_offset, increment) when increment > 0 do | ||
| %{log_offset | tx_offset: 1, op_offset: increment - 1} | ||
| end |
There was a problem hiding this comment.
I personally don't mind this too much, but would like to see a doctest for this case in particular
the ♾️ is really just a stand-in for the "supremum" of a given tx_offset, not an actual infinity, so adding one makes sense in that it moves to the next tx_offset. I see the point in calling this next but probably worth doing separately in a single PR.
That being said, I think I would prefer if this was also overspecified to the last_before_real_offsets which has tx_offset of 0 rather than any tx_offset - the only times we use infinity is for the supremum of the virtual offsets (which can move to the real offsets), and the last of the real offsets, which should not move on to the next tx offset.
packages/sync-service/lib/electric/shape_cache/pure_file_storage.ex
Outdated
Show resolved
Hide resolved
packages/sync-service/lib/electric/shapes/consumer/move_handling.ex
Outdated
Show resolved
Hide resolved
robacourt
left a comment
robacourt
left a comment
There was a problem hiding this comment.
Nice. I like the separation of concerns. I have left some comments on naming
packages/sync-service/lib/electric/shapes/consumer/initial_snapshot_state.ex
Outdated
Show resolved
Hide resolved
packages/sync-service/lib/electric/shapes/consumer/move_handling_state.ex
Outdated
Show resolved
Hide resolved
icehaunter
force-pushed
the
ilia/pr/rmysvxopsvsz
branch
4 times, most recently
from
7af481b to
4c05799
Compare
icehaunter
force-pushed
the
ilia/pr/rmysvxopsvsz
branch
4 times, most recently
from
ed304ff to
8245f1f
Compare
made sure other storages match the behaviour test fixes further composite key fixes fixes for 3-layer shapes added some unit tests addressing feedback
icehaunter
force-pushed
the
ilia/pr/rmysvxopsvsz
branch
from
8245f1f to
bda1aa9
Compare
icehaunter
force-pushed
the
ilia/pr/rmysvxopsvsz
branch
from
bda1aa9 to
e1473b8
Compare
Closes #3330