Repository navigation
Conversation
|
Review changes with |
Contributor
No docs changes detected for 2d4684a |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The test target lacks an explicit Immer dependency, preventing compilation when dependency headers use separate installation prefixes.
1 open issue
What changed in this PR
This PR fixes C++ subscription handling when the server re-sends rows during a multi-round initial snapshot. It builds on the preceding fixes for partial snapshot delivery and removal of undelivered rows.
Changes:
- Size incoming additions from included rows and represent re-sent rows as removals followed by additions.
- Add held-key filtering and combine successive erasures into original row positions.
- Add unit and live-server regression tests for snapshot delivery.
| File | Description |
|---|---|
cpp-client/deephaven/tests/src/table_state_test.cc |
Tests partial erasures, held-key filtering, and combined erasures. |
cpp-client/deephaven/tests/src/growing_subscription_test.cc |
Exercises multi-round snapshots against a live server. |
cpp-client/deephaven/tests/src/barrage_processor_test.cc |
Tests snapshot message handling and re-sent rows. |
cpp-client/deephaven/tests/include/private/deephaven/tests/barrage_test_util.h |
Adds shared row and column test helpers. |
cpp-client/deephaven/tests/CMakeLists.txt |
Registers tests and exposes required private headers. |
cpp-client/deephaven/dhcore/src/ticking/space_mapper.cc |
Implements held-key filtering and tolerant position mapping. |
cpp-client/deephaven/dhcore/src/ticking/immer_table_state.cc |
Skips unheld removals and combines erasure positions. |
cpp-client/deephaven/dhcore/src/ticking/barrage_processor.cc |
Handles included rows and replaces re-sent rows. |
cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/space_mapper.h |
Declares held-key mapping helpers. |
cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/immer_table_state.h |
Declares held-key lookup and erasure-combination helpers. |
Review effort: Balanced
kosak
force-pushed
the
kosak_cpp-barrage-fix-re-sends
branch
from
November 2, 2026 23:23
63c7ff1 to
8348c9c
Compare
kosak
force-pushed
the
kosak_cpp-barrage-fix-re-sends
branch
from
November 3, 2026 00:18
8348c9c to
e8a96a1
Compare
kosak
force-pushed
the
kosak_cpp-barrage-fix-re-sends
branch
from
November 3, 2026 00:42
e8a96a1 to
3ae979d
Compare
kosak
force-pushed
the
kosak_cpp-barrage-fix-re-sends
branch
from
November 4, 2026 17:29
3ae979d to
c1b1c4d
Compare
kosak
added a commit
that referenced
this pull request
Nov 4, 2026
…_included (#8997) C++ port of #8994. First of three stacked PRs (#8998 and #8999 follow) fixing the C++ client's handling of a ticking subscription while the initial snapshot of a large table is still being delivered in rounds; the C++ client has the same gaps the C# client had. The subscription processor sized its add phase from `added_rows` and never read `added_rows_included`. The two differ only during that initial snapshot: a snapshot round announces the table's whole row set in `added_rows` but carries data only for its own slice, and a delta between rounds can carry data for rows `added_rows` does not mention (rows that moved into the already-delivered region) and none for rows it does (rows a later round will deliver). Sized from the wrong set, the add phase kept consuming the record batches of later messages, which surfaced as "There is excess data in the chunk" on a large appending table and "Chunks have inconsistent sizes" on a wide table whose rows are modified every cycle. `AwaitingMetadata::ProcessNextChunk` now sizes the add phase, and inserts keys, from `added_rows_included`, defaulting to `added_rows` when the flatbuffer field is absent. The server omits it exactly when the two are identical, which is every message of an established subscription, so this mirrors the Java client's reader defaulting `rowsIncluded` to `rowsAdded`. A row that `added_rows` announces without data is added later, when the round carrying its data arrives. Key insertion stays strict: during a growing snapshot the server can legitimately re-send a row this client already holds, and for now such a row fails loudly inside `SpaceMapper` rather than being silently inserted twice. Handling it as an overwrite is a separate change (#8999); the "re-sent row is rejected" case pins the current behavior so the hand-off is explicit. The same holds for removes: a full subscription is sent the server's entire removed set, which during a growing snapshot can name rows this client has not been given yet. For now such a remove fails loudly inside `SpaceMapper`; skipping those rows is the next change (#8998), and the "remove of undelivered rows is rejected" case pins the hand-off. Tests: `barrage_processor_test` drives `BarrageProcessor` without a server, feeding hand-built `BarrageUpdateMetadata` flatbuffers and record batches one at a time, with a test-side encoder for the row-set wire format. It builds that metadata from the flatbuffer schema dhcore vendors, so the tests target gains `dhcore/flatbuf` and the vendored flatbuffers headers on its include path. The cases cover a snapshot round sized from the included set, included rows spanning record batches, later rounds re-announcing the whole row set, a delta whose included set is empty, a row scoped in with empty `added_rows`, and an absent field meaning "same as `added_rows`", and, for DH-24154, slices arriving out of key order. `growing_subscription_test` subscribes to the two tables from the bug report against a live server. The part of a table that changes is an input table, empty when the subscription opens; the test waits for the first snapshot round to arrive and then adds rows to it with a second script, so that the delta is applied while the rest of the snapshot is still being delivered, and it asserts that the delta was observed. A column computed by a Python function that sleeps a few microseconds per row keeps the rounds small: the server sizes a round to about a quarter of an update graph cycle, so the snapshot spans several cycles and the delta has room to land between rounds. Verified at this commit with `./gradlew :cpp-client:testCppClient`: 99 test cases, 649 assertions, 0 failures --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…scription has not received These fail against the current client, which converts every removed key strictly and throws out of SpaceMapper when one is not held. A full subscription is sent the server's entire removed set, and while the initial snapshot of a large table is still arriving in rounds, that set can name rows this client has not been given yet. table_state_test pins down ImmerTableState::Erase against a table holding a known set of keys; dhcore's private headers are now on the tests' include path for it, which is safe because dhcore exports every symbol, and the tests find Immer for themselves because dhcore links it privately. barrage_processor_test replaces the test that pinned the strict rejection with the two message shapes: a removed set entirely beyond the delivered region, which must leave the table untouched and report that the way an empty removed set does, and one straddling it, which must erase only the held rows. growing_subscription_test subscribes to the table from the bug report, whose rows are removed from the top of the key space while the snapshot delivers it from the bottom. The test issues the removes itself once the first round has arrived, a few from the bottom so that the delta is visible to the client and many from the top, and checks that it arrived while the rows from the top were still undelivered. The helpers the server-free tests share move into barrage_test_util.h. Ports of the C# tests for DH-24171. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…not received A full subscription is sent the server's entire removed set, and while the initial snapshot of a large table is still arriving in rounds, that set can name rows this client has not been given yet. ImmerTableState::Erase converted every removed key strictly, so such a message threw out of SpaceMapper, and even a partially held interval would have sliced every column by the interval's full width rather than by the rows actually held. SpaceMapper gains ConvertPresentKeysToIndices, which maps each removed interval to the positions of only the keys it holds; within one interval those occupy consecutive ranks, so each interval still maps to one range. Erase uses it, slices each column by the ranks held in each interval, and does nothing at all when none of the named rows is held. ProcessRemoves then treats a removed set that erased nothing like an empty one, handing out the same table before and after so that consumers relying on pointer equality still see "unchanged". This is the C++ port of the C# fix for DH-24171. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ring the initial snapshot These fail against the current client, which inserts every included row strictly and throws out of SpaceMapper when one is already held: "Some elements of [...] were already in the set". While the initial snapshot of a large table is still arriving in rounds, the server tracks what the client holds as a range of positions. A row inserted ahead of the delivered region pushes the last delivered row out of that range, and the next round covers that row again, sending its data to a client that already has it. The Java client treats such a row as an overwrite. barrage_processor_test replaces the test that pinned the strict rejection with two that describe the intended result: the re-sent row is reported as removed and then added back carrying the new data, and when the same message also removes rows, the reported positions are those of the table before the message. growing_subscription_test subscribes to a table whose inserted rows take the lowest row keys, so that they land ahead of the snapshot. The test inserts them itself once the first round has arrived, and checks that each delivered row they pushed out of the server's range was sent again. Ports of the C# tests for DH-24171. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… initial snapshot While the initial snapshot of a large table is still arriving in rounds, the server tracks what the client holds as a range of positions. A row inserted ahead of the delivered region pushes the last delivered row out of that range, and the next round covers that row again, sending its data to a client that already holds it. The client inserted every included row strictly and threw: "Some elements of [...] were already in the set". The Java client treats such a row as an overwrite. This client holds only rows it has data for, so it takes the row out and adds it back with the new data, which reports it to the user as removed and then added. That is the one representation in which every before-and-after table in the update agrees with the index sets that describe it; overwriting in place during the add phase would change rows between AfterRemoves and AfterAdds that AddedRows does not mention. AwaitingMetadata now erases the removed rows, applies the shifts, hands the whole included set to Erase, which skips the rows it does not hold and so takes out exactly the re-sent ones, and only then adds keys for the included rows, which stays strict. The second erasure happens in the index space of the table after the first, so ImmerTableState::CombineErasures lifts its positions past the rows the first one took out before the two are reported as one removed set. With Erase now a no-op for an empty or entirely unheld set, the special cases in ProcessRemoves are gone and it is folded into the main flow. This is the C++ port of the C# fix for DH-24171. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kosak
force-pushed
the
kosak_cpp-barrage-fix-re-sends
branch
from
November 4, 2026 19:50
c1b1c4d to
2d4684a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is stacked on top of #8998. It will get simpler once that PR has been merged.
C++ port of #8996, and last of the three. While the initial snapshot of a large table is still arriving in rounds, the server tracks what the client holds as a range of positions. A row inserted ahead of the delivered region pushes the last delivered row out of that range, and the next round covers that row again, sending its data to a client that already holds it. The client inserted every included row strictly (the behavior #8997 deliberately pinned) and threw: "Some elements of [...] were already in the set".
The Java client treats such a row as an overwrite. This client holds only rows it has data for, so it takes the row out and adds it back with the new data, which reports it to the user as removed and then added. That is the one representation in which every before-and-after table in the update agrees with the index sets that describe it: overwriting in place during the add phase would change rows between
AfterRemovesandAfterAddsthatAddedRowsdoes not mention. The obvious shortcut, letting key insertion tolerate an existing key, is wrong here for a different reason:AddDatainserts rather than overwrites, so the row would end up held twice.AwaitingMetadata::ProcessNextChunknow erases the removed rows, applies the shifts, hands the whole included set toErase, which skips the rows it does not hold (from #8998) and so takes out exactly the re-sent ones, and only then adds keys for the included rows, which stays strict. The second erasure happens in the index space of the table after the first, soImmerTableState::CombineErasureslifts its positions past the rows the first one took out before the two are reported as one removed set in the index space of the original table. WithErasenow a no-op for an empty or entirely unheld set (from #8998), the special cases inProcessRemovesare gone and it is folded into the main flow; the method is removed.Tests:
barrage_processor_testreplaces "re-sent row is rejected" with two cases describing the intended result: the re-sent row is reported as removed and then added back carrying the new data, and when the same message also removes rows, the reported positions are those of the table before the message.table_state_testcoversCombineErasures(the second erasure's positions count only the rows that survived the first, so each has to move past them).growing_subscription_testadds a table whose inserted rows take the lowest row keys, so that they land ahead of the snapshot. The test inserts them itself once the first round has arrived, and checks that each delivered row they pushed out of the server's range was sent again.Verified at this commit with
./gradlew :cpp-client:testCppClient: 107 test cases, 691 assertions, 0 failures