fix: DH-22012: implement barrage bug fixes#7800
fix: DH-22012: implement barrage bug fixes#7800lbooker42 wants to merge 6 commits intodeephaven:mainfrom
Conversation
No docs changes detected for 502c7dd |
lbooker42
added
NoDocumentationNeeded
ReleaseNotesNeeded
There was a problem hiding this comment.
Pull request overview
Implements targeted Barrage-related bug fixes across server propagation scheduling, client redirected table update handling, and test infrastructure to surface delayed error notifications.
Changes:
- Fix propagation rescheduling condition in
BarrageMessageProducerto use the configured update interval. - Adjust
BarrageRedirectedTableupdate processing to skip coalescing when the downstreamTableUpdateis empty (and remove a snapshot-specific early-return path). - Extend
RefreshingTableTestCaseto build a customUpdateGraphthat detectsDelayedErrorNotifierscheduling and fails tests if any are generated.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/src/main/java/io/deephaven/server/barrage/BarrageMessageProducer.java | Corrects the time-gap comparison used when deciding whether to delay propagation scheduling. |
| extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageRedirectedTable.java | Changes snapshot/update handling and adds an early return when the computed downstream update is empty. |
| engine/test-utils/src/main/java/io/deephaven/engine/testutil/testcase/RefreshingTableTestCase.java | Introduces a custom execution context/update graph to count delayed error notifier registrations and fail in tearDown(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| return coalescer; | ||
| } | ||
|
|
||
| return (coalescer == null) ? new UpdateCoalescer(currRowsFromPrev, downstream) | ||
| : coalescer.update(downstream); |
There was a problem hiding this comment.
Agree, totalMods can leak.
There was a problem hiding this comment.
It would be preferable to try w/ resources for it. Downstream owns it once we pass it in; but the downstream is going to be maybe not relevant and the coalescer copies that. So I think a better pattern might be to close downstream, but we need to make sure it actually owns the things that it was passed.
There was a problem hiding this comment.
looked into having the coalescer auto-release the downstream update, but another bit of code in BMP uses this class to process delta updates and requires the passed-in update rowsets to be available after these calls (constructor / update() ).
Decided to try-with-resources to clean up this call-site and not re-write the UpdateCoalescer code.
.../test-utils/src/main/java/io/deephaven/engine/testutil/testcase/RefreshingTableTestCase.java
Show resolved
Hide resolved
.../test-utils/src/main/java/io/deephaven/engine/testutil/testcase/RefreshingTableTestCase.java
Outdated
Show resolved
Hide resolved
| return coalescer; | ||
| } | ||
|
|
||
| return (coalescer == null) ? new UpdateCoalescer(currRowsFromPrev, downstream) | ||
| : coalescer.update(downstream); |
There was a problem hiding this comment.
It would be preferable to try w/ resources for it. Downstream owns it once we pass it in; but the downstream is going to be maybe not relevant and the coalescer copies that. So I think a better pattern might be to close downstream, but we need to make sure it actually owns the things that it was passed.
Implements targeted Barrage-related bug fixes across server propagation scheduling, client redirected table update handling, and test infrastructure to surface delayed error notifications.
Changes:
BarrageMessageProducerto use the configured update interval.BarrageRedirectedTableupdate processing to skip coalescing when the downstreamTableUpdateis empty (and remove a snapshot-specific early-return path).RefreshingTableTestCaseto build a customUpdateGraphthat detectsDelayedErrorNotifierscheduling and fails tests if any are generated.