feat: Simplify Barrage Viewport Table Updates #6347

nbauernfeind · 2024-11-08T17:25:21Z

Fixes #6039.
Fixes #6053.

The barrage viewport protocol has now changed:

viewports are never sent included rows; all added rows are included
added rows are position space from within the server-respected client requested viewport
removed rows are position space from within the server-respected client requested viewport
snapshots on viewports include removed rows to simplify client side processing
barrage update metadata now explicitly includes current table size; viewports no longer track the source table's full rowset

viewport clients must infer shifts:

removed rows are in pre-update space
added rows are in post-update space
the end-result viewport is retained rows + added rows (retained are the existing rows minus removed rows) where the adds are in the offsets specified by added rows

In this PR we'll bump the following dependencies:

flatbuffer from 1.12.0 to 24.3.25
arrow from 13.0.0 to 18.0.0
protobuf 3.25.3 to 3.25.4
barrage from 0.6.0 to 0.7.2 (noting that 0.7.x was released for this feature set)

flakiness (#9948)"

response (#9314)"

…lification

buildSrc/build.gradle

.../barrage/src/main/java/io/deephaven/extensions/barrage/chunk/DefaultChunkReadingFactory.java

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/util/StreamReaderOptions.java

...nsions/barrage/src/main/java/io/deephaven/extensions/barrage/BarrageSubscriptionOptions.java

...r/src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java

java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSnapshotImpl.java

server/src/main/java/io/deephaven/server/barrage/BarrageMessageProducer.java

...r/src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java

rcaudy · 2024-11-15T23:17:45Z

...r/src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java


-        prevKeyspaceViewportRows.clear();
-        prevKeyspaceViewportRows.insert(keyspaceViewportRows);
+            prevKeyspaceViewportRows.resetTo(barrageMessage.rowsIncluded);


I think getSubView needs to document that (1) it does not own the row sets passed in, and (2) when they can be mutated.
The keyspace viewports can be mutated as soon as the call returns.
The position space viewport must not be mutated until the message is sent.

rcaudy

.

rcaudy

LGTM

niloc132

Doesnt look like we have a formal ticket for this?

#6053 would be a good one.

Also please add notes to commit message/description about barrage/arrow/flatbuffer vers bumps.

...nt-api/src/main/java/io/deephaven/web/client/api/subscription/TableViewportSubscription.java

server/test-utils/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java

java-client/flight-examples/src/main/java/io/deephaven/client/examples/DoExchange.java

.../barrage/src/main/java/io/deephaven/extensions/barrage/chunk/DefaultChunkReadingFactory.java

server/src/main/java/io/deephaven/server/barrage/BarrageMessageProducer.java

...r/src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java

server/src/test/java/io/deephaven/server/barrage/BarrageMessageRoundTripTest.java

...lient-api/src/main/java/io/deephaven/web/client/api/barrage/data/WebBarrageSubscription.java

niloc132

Two remarks, neither blocking, so I approved.

In addition to the other remark, I'll follow up with bumping required java versions to the downstream modules of arrow 18, as it has a minimum of Java 11 now, and so do all of our libraries that depend on it, directly or indirectly.

niloc132 · 2024-11-18T14:02:14Z

web/shared-beans/src/main/java/io/deephaven/web/shared/data/RangeSet.java

@@ -545,6 +549,31 @@ public boolean includesAnyOf(Range range) {
        return range.getFirst() <= target.getLast() && range.getLast() >= target.getFirst();
    }

+    public long find(long key) {


i'll follow up this PR with unit tests

...lient-api/src/main/java/io/deephaven/web/client/api/barrage/data/WebBarrageSubscription.java

deephaven-internal · 2024-11-18T16:39:53Z

Labels indicate documentation is required. Issues for documentation have been opened:

Community: deephaven/deephaven-docs-community#362

niloc132 and others added 12 commits October 21, 2024 13:34

Restore initial commit from grpc-java, plus a few local changes

1852b0a

Guard writing payload as hex if FINEST is enabled

68b925a

Apply upstream "Fix AsyncServletOutputStreamWriterConcurrencyTest

e88c47e

flakiness (#9948)"

Apply upstream "Avoid flushing headers when the server returns a single

f9a19fc

response (#9314)"

Apply upstream "servlet: introduce ServletServerBuilder.buildServlet()"

4733524

Bump grpc vers, add inprocess dep for tests

06e63ec

Merge branch 'main' into grpc-history-replay

09ade64

Apply deephaven#6301

c8af47c

Bump to 1.65.1 to better match arrow 18

57c8008

Merge remote-tracking branch 'colin/grpc-history-replay' into vp_simp…

cbf8ab2

…lification

Version Upgrades; MavenLocal

85f604f

Implement Simplified Viewport Table Updates in BMP/BT

70a0207

nbauernfeind added jsapi barrage core Core development tasks DocumentationNeeded barrage-wrkr2wrkr ReleaseNotesNeeded Release notes are needed labels Nov 8, 2024

nbauernfeind requested review from niloc132 and rcaudy November 8, 2024 17:25

nbauernfeind self-assigned this Nov 8, 2024

nbauernfeind marked this pull request as draft November 8, 2024 17:25

niloc132 mentioned this pull request Nov 8, 2024

chore(gradle): bump com.vertispan.flatbuffers:flatbuffers-gwt from 1.12.0-1 to 24.3.25-1 #6352

Closed

rcaudy reviewed Nov 8, 2024

View reviewed changes

nbauernfeind added 6 commits November 8, 2024 17:10

Ryan's Synchronous Review

0089d62

Merge remote-tracking branch 'upstream/main' into vp_simplification

485746d

Remove SNAPSHOT version and mavenLocal references

ad8de73

Fixes removed/added rows in most VP cases

02ce2ad

Bug fixes around viewport snapshot rowsRemoved and rowsAdded

da23e2b

Bugfix for correct growing VP logic

299f56e

nbauernfeind added 3 commits November 15, 2024 13:11

Fixes for jsapi and HierarchicalTable

d549d79

Lazily compute rowset encoding

b4d5b69

Fixup jsapi tests

6c12314

nbauernfeind marked this pull request as ready for review November 15, 2024 21:19

nbauernfeind mentioned this pull request Nov 15, 2024

BarrageStreamGeneratorImpl#getSubView Should Be Bifurcated into FullSubscriptionView and ViewportSubscriptionView #6383

Open

nbauernfeind added 3 commits November 15, 2024 14:57

Quick round feedback

f9be6e5

spotless

4252622

Double checked locking fixes

2767def

rcaudy reviewed Nov 15, 2024

View reviewed changes

Ryan's final review

78c4cb7

rcaudy reviewed Nov 15, 2024

View reviewed changes

nbauernfeind added 2 commits November 15, 2024 16:22

Clarify strategy on who owns RowSets passed into getSubView

ea6f898

npe fix

3eeb628

rcaudy reviewed Nov 15, 2024

View reviewed changes

niloc132 reviewed Nov 16, 2024

View reviewed changes

nbauernfeind added 2 commits November 15, 2024 17:54

Bugfix if HT is empty or viewport past end of table

84a6100

Colin's feedback

476ae65

rcaudy previously approved these changes Nov 16, 2024

View reviewed changes

Limit jsapi data change event to prev and curr table sizes

738cb11

nbauernfeind dismissed rcaudy’s stale review via 738cb11 November 16, 2024 08:54

niloc132 self-requested a review November 18, 2024 13:59

niloc132 mentioned this pull request Nov 18, 2024

chore(gradle): bump deephaven-barrage from 0.6.0 to 0.7.2 #6388

Closed

niloc132 previously approved these changes Nov 18, 2024

View reviewed changes

Colin's Final Feedback

44fadff

nbauernfeind dismissed niloc132’s stale review via 44fadff November 18, 2024 15:55

niloc132 approved these changes Nov 18, 2024

View reviewed changes

nbauernfeind merged commit d3b0ad4 into deephaven:main Nov 18, 2024
17 checks passed

github-actions bot locked and limited conversation to collaborators Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Simplify Barrage Viewport Table Updates #6347

feat: Simplify Barrage Viewport Table Updates #6347

nbauernfeind commented Nov 8, 2024 •

edited

Loading

rcaudy Nov 15, 2024

rcaudy left a comment

rcaudy left a comment

niloc132 left a comment

niloc132 left a comment

niloc132 Nov 18, 2024

deephaven-internal commented Nov 18, 2024

feat: Simplify Barrage Viewport Table Updates #6347

feat: Simplify Barrage Viewport Table Updates #6347

Conversation

nbauernfeind commented Nov 8, 2024 • edited Loading

rcaudy Nov 15, 2024

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

niloc132 left a comment

Choose a reason for hiding this comment

niloc132 left a comment

Choose a reason for hiding this comment

niloc132 Nov 18, 2024

Choose a reason for hiding this comment

deephaven-internal commented Nov 18, 2024

nbauernfeind commented Nov 8, 2024 •

edited

Loading