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.

Sign up for GitHub

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

Enable Support for Multiple Update Graph Processors #3506

Merged

rcaudy merged 87 commits into deephaven:main from nbauernfeind:multi_ugp

Jun 3, 2023

Member

nbauernfeind commented Mar 8, 2023 •

edited

Loading

Here is some sample Groovy usage:

import io.deephaven.util.SafeCloseable
import io.deephaven.engine.util.TableTools
import io.deephaven.engine.updategraph.impl.PeriodicUpdateGraph
import io.deephaven.engine.context.ExecutionContext

oobUpdateGraph = PeriodicUpdateGraph.newBuilder("OOB").numUpdateThreads(1).build()
try (SafeCloseable ignored = ExecutionContext.getContext().withUpdateGraph(oobUpdateGraph).open()) {
    t = TableTools.timeTable("PT00:00:00.1").tail(1)
}

Observe that the table still ticks when the REPL is holding the exclusive DEFAULT PUG lock:

Thread.sleep(20000)
println("done")

Note that as-is the UGP stops and does not propagate a stop-related error notification to source table listeners. We might want to do something to be clear that the tables are done. We cannot purely set reference count to zero as this does not propagate downstream. I did try out such a change, but it some tests that were not expecting liveness referents / artifacts to fail manage calls.

Nightlies: https://github.com/nbauernfeind/deephaven-core/actions/runs/4789971295

nbauernfeind added query engine DocumentationNeeded enhancement ReleaseNotesNeeded labels

nbauernfeind added this to the Mar 2023 milestone

nbauernfeind self-assigned this

nbauernfeind requested review from chipkent, jmao-denver and rcaudy as code owners

March 8, 2023 05:44

devinrsmith reviewed

View reviewed changes

Generators/Generators.gradle Outdated Show resolved Hide resolved

Member

devinrsmith commented Mar 8, 2023

I haven't reviewed this PR, but I have done a quick migration of io.deephaven.app.GcApplication to use its own UGP; I was able to verify that the GcApplication was able to tick while the main UGP was busy.

nbauernfeind modified the milestones: Mar 2023, Apr 2023

chipkent previously approved these changes

View reviewed changes

nbauernfeind dismissed chipkent’s stale review via

706d73c

April 18, 2023 00:30

nbauernfeind force-pushed the multi_ugp branch 3 times, most recently from eee5c0a to 90a2730 Compare

April 18, 2023 02:49

chipkent previously approved these changes

View reviewed changes

nbauernfeind dismissed chipkent’s stale review via

9e18f5d

April 24, 2023 18:48

nbauernfeind force-pushed the multi_ugp branch from 90a2730 to 9e18f5d Compare

April 24, 2023 18:48

rcaudy reviewed

View reviewed changes

Member

rcaudy left a comment

High level review. Suggested some refactoring, and a change to the policy that determines UGP for derived tables.

engine/updategraph/src/main/java/io/deephaven/engine/updategraph/UpdateContext.java Outdated Show resolved Hide resolved

engine/updategraph/src/main/java/io/deephaven/engine/updategraph/UpdateContext.java Outdated Show resolved Hide resolved

engine/updategraph/src/main/java/io/deephaven/engine/updategraph/UpdateContext.java Outdated Show resolved Hide resolved

engine/updategraph/src/main/java/io/deephaven/engine/updategraph/UpdateContext.java Outdated Show resolved Hide resolved

engine/updategraph/src/main/java/io/deephaven/engine/updategraph/UpdateContext.java Outdated Show resolved Hide resolved

engine/api/src/main/java/io/deephaven/engine/table/Table.java Outdated Show resolved Hide resolved

engine/api/src/main/java/io/deephaven/engine/table/Table.java Outdated Show resolved Hide resolved

engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java Outdated Show resolved Hide resolved

server/src/main/java/io/deephaven/server/table/ops/JoinTablesGrpcImpl.java Outdated Show resolved Hide resolved

server/src/main/java/io/deephaven/server/table/ops/GrpcTableOperation.java Outdated Show resolved Hide resolved

Member

rcaudy commented May 8, 2023

We discussed in-person about maybe just having the idea of an UpdateGraph object that hides the "processor" details, with a clock(), sharedLock(), exclusiveLock(), and other appropriate interfaces exposed. LogicalClock should probably have an interface that does not expose the mutating methods.

Member

mofojed commented May 30, 2023

@rcaudy Is this targeting June release now?

nbauernfeind force-pushed the multi_ugp branch 3 times, most recently from 23c89d8 to 7b5320c Compare

June 1, 2023 18:21

rcaudy force-pushed the multi_ugp branch 2 times, most recently from ce90980 to aeedeb9 Compare

June 2, 2023 00:09


Enable Support for Multiple Update Graph Processors

d535d15

rcaudy and others added 9 commits

June 2, 2023 22:49


Fix even more tests

8d994c3


For now re-use the ControlledUpdateGraph

010cc76


Fix FlightMessageRoundTripTests

b3a46fd


cleaner fix on api server test base

eb6dc3b


revert removal of livenessScope in ApiServerTestBase

8adea9d


Some python fixes, and experimental improvements to DeephavenApiServe…

cb33689

…rTestBase and TestExecutionContext


This might enable simple static use cases

d9a193a


Fix the remaining Python issues, I think, and fix some JavaDoc format…

e983c06

…ting


Merge remote-tracking branch 'nate/multi_ugp' into multi_ugp

nbauernfeind added the breaking label

rcaudy previously approved these changes

View reviewed changes

Member

rcaudy left a comment

Approving. Please don't merge unless nightlies are green.

chipkent requested changes

View reviewed changes

py/server/deephaven/execution_context.py

@@ -36,6 +37,10 @@ class ExecutionContext(JObjectWrapper, ContextDecorator):

 def j_object(self) -> jpy.JType:

 return self.j_exec_ctx

 @property

 def update_graph(self) -> _JUpdateGraph:

Member

chipkent Jun 3, 2023

Two problems:

The API is exposing Java objects to the user.
The type hint is referring to a private variable.

py/server/deephaven/table.py

@@ -537,6 +539,15 @@ def is_refreshing(self) -> bool:

 self._is_refreshing = self.j_table.isRefreshing()

 return self._is_refreshing

 @property

 def update_graph(self) -> _JUpdateGraph:

Member

chipkent Jun 3, 2023

Two problems:

The API is exposing Java objects to the user.
The type hint is referring to a private variable.

py/server/deephaven/table.py

@@ -2298,6 +2309,11 @@ def table(self) -> Table:

 self._table = Table(j_table=self.j_partitioned_table.table())

 return self._table

 @property

 def update_graph(self) -> _JUpdateGraph:

Member

chipkent Jun 3, 2023

Two problems:

The API is exposing Java objects to the user.
The type hint is referring to a private variable.

py/server/deephaven/table.py

@@ -2554,6 +2570,11 @@ def is_refreshing(self) -> bool:

 """Whether this proxy represents a refreshing partitioned table."""

 return self.target.is_refreshing

 @property

 def update_graph(self) -> _JUpdateGraph:

Member

chipkent Jun 3, 2023

Two problems:

The API is exposing Java objects to the user.
The type hint is referring to a private variable.

py/server/deephaven/table_listener.py

@@ -237,25 +238,29 @@ def modified_columns(self) -> List[str]:

 return list(cols) if cols else []

def _do_locked(f: Callable, lock_type="shared") -> None:

 """Executes a function while holding the UpdateGraphProcessor (UGP) lock. Holding the UGP lock

def _do_locked(ug: Union[_JUpdateGraph, Table], f: Callable, lock_type="shared") -> None:

Member

chipkent Jun 3, 2023

Two problems:

The API is exposing Java objects to the user.
The type hint is referring to a private variable.

py/server/deephaven/update_graph.py

be released after the table operation finishes. Auto locking is turned on by default."""

def has_exclusive_lock(ug: Union[_JUpdateGraph, "Table", "PartitionedTable", "PartitionTableProxy"]) -> bool:

Member

chipkent Jun 3, 2023

Two problems:

The API is exposing Java objects to the user.
The type hint is referring to a private variable.

Member

chipkent Jun 3, 2023

Many places in this file.

chipkent previously approved these changes

View reviewed changes


1. Make DeephavenApiServerTestBase more resilient for subsequent test…

61ae914

…s after failure

2. Add PeriodicUpdateGraph caching by name, and re-use when providing via Dagger
3. Add proper unit test for update graph conflicts
4. Fix bug in update graph provision for merge and partitioned table construction

rcaudy dismissed stale reviews from chipkent and themself via

61ae914

June 3, 2023 14:16


Clean up python session initialization for tests, and fix bug in sess…

32e6854

…ion initializationf or sphinx

rcaudy previously approved these changes

View reviewed changes

rcaudy enabled auto-merge (squash)

June 3, 2023 14:45

rcaudy disabled auto-merge

June 3, 2023 14:45

rcaudy enabled auto-merge (squash)

June 3, 2023 14:48

rcaudy dismissed their stale review

June 3, 2023 14:48

Waiting on nightlies

rcaudy and others added 3 commits

June 3, 2023 11:10


Re-fix the Python tests

0c79352


Must install UpdateGraph in Session ThreadFactory Threads

f0ee72f


Map#put returns old value not new value

29d7e81

nbauernfeind mentioned this pull request

Python Server: Minimally Wrap UpdateGraph #3925

Closed

rcaudy approved these changes

View reviewed changes

rcaudy merged commit ec85c5b into deephaven:main

github-actions bot locked and limited conversation to collaborators

Contributor

deephaven-internal commented Jun 3, 2023

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

How-to: https://github.com/deephaven/deephaven.io/issues/2731
Conceptual: https://github.com/deephaven/deephaven.io/issues/2728
Reference: https://github.com/deephaven/deephaven.io/issues/2730
Blog: https://github.com/deephaven/deephaven.io/issues/2729

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking DocumentationNeeded enhancement query engine ReleaseNotesNeeded