Add java client TableServiceAsync #4756

devinrsmith · 2023-11-01T17:51:52Z

No description provided.

…ort state management

java-client/session/src/main/java/io/deephaven/client/impl/Session.java

java-client/session/src/main/java/io/deephaven/client/impl/TableService.java

java-client/session/src/main/java/io/deephaven/client/impl/TableServiceAsync.java

java-client/session/src/main/java/io/deephaven/client/impl/TableServiceAsyncImpl.java

rcaudy · 2023-11-03T16:56:42Z

java-client/session/src/main/java/io/deephaven/client/impl/ExportServiceRequest.java

+interface ExportServiceRequest extends Closeable {
+    List<Export> exports();
+
+    // Note: not providing cancel() or Runnable as return here; we don't really want to cancel a Batch request, we'll


Wouldn't trying to cancel the batch maybe be better on the server sometimes?

I think we can debate this point once we hook up the on cancel handlers for the server, after the semantics of said cancel is clearly document.

rcaudy · 2023-11-03T17:04:15Z

java-client/session/src/main/java/io/deephaven/client/impl/ExportStates.java


-        return results;
+            @Override
+            public void close() {


We have the potential for state leak if the user calls close without calling send. Do we need to clean those up? What about on errors?

Good catch. I've added some new logic to handle the case where the user does not call send. After sent, any errors are the handled by respective io.deephaven.client.impl.ExportRequest.listener.

rcaudy · 2023-11-03T17:08:52Z

java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java

-    @Override
-    public TableHandleManager serial() {
-        return serialManager;
+    protected TableServices delegate() {


So the pattern is we construct a whole bunch of state, and then throw it away...

Yes. The internal state is needed even for a single query - there may be shared sub-DAGs that we don't want to export multiple times in a single Batch request. One might argue "we should re-implement all of this logic, only keeping the state/Map alive for the duration of the execute method", but re-working as such would add additional maintenance burdens for the sake of saving the construction of a extra object, which is not even close to worth it IMO.

…ce-async

server/src/main/java/io/deephaven/server/session/SessionState.java

rcaudy · 2023-11-06T19:19:33Z

java-client/session/src/main/java/io/deephaven/client/impl/TableService.java

+    interface TableHandleFuture extends Future<TableHandle> {
+
+        /**
+         * Waits if necessary for the computation to complete, and then retrieves its result. If an


This JavaDoc is trying to describe a somewhat complex set of branching logic, but the actual branches are hard to understand from reading the doc.

rcaudy · 2023-11-06T19:33:35Z

java-client/session/src/main/java/io/deephaven/client/impl/FutureHelper.java

+        }
+    }
+
+    static <T> void cancelOrConsume(Iterable<? extends Future<T>> futures, BiConsumer<T, ExecutionException> consumer,


This class could generally use some JavaDoc, but cancelOrConsume is the weirdest. Maybe a "tri consumer" is more appropriate, for CancellationExceptions?

FutureConsumer works nicely.

I added some javadoc to cancelOrConsume b/c it's easy, but I don't know how to easily document getOrCancel without getting into the weeds of the exact implementation.

java-client/session/src/main/java/io/deephaven/client/impl/TableService.java

java-client/session/src/main/java/io/deephaven/client/impl/TableHandleManagerBatch.java

java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java

java-client/session-examples/src/main/java/io/deephaven/client/examples/FilterTable.java

...session-examples/src/main/java/io/deephaven/client/examples/UnreferenceableTableExample.java

rcaudy · 2023-11-06T20:03:50Z

java-client/session/src/main/java/io/deephaven/client/impl/ExportStates.java

@@ -224,7 +261,7 @@ class State {
        State(TableSpec table, int exportId) {
            this.table = Objects.requireNonNull(table);
            this.exportId = exportId;
-            this.children = new LinkedHashSet<>();
+            this.children = new CopyOnWriteArraySet<>();


I understand we want this to avoid CME's on remove during an iteration. Can we build a list to remove and do a removeAll instead? I'm worried about adding COW data structures with user-facing adds.

I more thoroughly traced exactly the part that was causing concurrent mod, and it was actually the mitigation; now that the Batch / ETCR race has been fixed, I can get rid of the mitigation and revert this change.

server/src/main/java/io/deephaven/server/util/Scheduler.java

…ce-async

deephaven-internal · 2023-11-07T16:23:05Z

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

How-to: https://github.com/deephaven/deephaven.io/issues/3399
Reference: https://github.com/deephaven/deephaven.io/issues/3400

devinrsmith added 9 commits October 27, 2023 11:38

Initial pass at making TableServiceAsync and being explicit about exp…

27b3268

…ort state management

Make ExportService internal

d083cd9

Delay sending BatchRequest

9c572c4

f

8a0079c

move non-public methods into TableServiceImpl

373e3d1

tests

a1e305c

Helpers around get and close

9597885

Merge remote-tracking branch 'upstream/main' into table-service-async

8eeb8c9

Mitigation for deephaven#4754

cfda32f

devinrsmith added grpc java-client labels Nov 1, 2023

devinrsmith added this to the November 2023 milestone Nov 1, 2023

devinrsmith self-assigned this Nov 1, 2023

devinrsmith added 2 commits November 1, 2023 12:32

add explicitness about cancel

e799295

Lower mitigation to 100 millis; change selects into views

a4ed50b

devinrsmith requested a review from rcaudy November 1, 2023 22:47

devinrsmith added ReleaseNotesNeeded Release notes are needed DocumentationNeeded labels Nov 1, 2023

devinrsmith marked this pull request as ready for review November 1, 2023 22:47

devinrsmith added 2 commits November 2, 2023 09:02

Add Scheduler shutdown logic for api server tests

8879935

Ensure the cleaner thread gets promptly cleaned up in unit tests

2f0261e

devinrsmith mentioned this pull request Nov 2, 2023

Unit-test thread cleanup #4764

Merged

Fix javadoc issue. Make package private.

adb9807

rcaudy reviewed Nov 3, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into nightly/table-servi…

fe42b00

…ce-async

devinrsmith requested a review from rcaudy November 6, 2023 16:11

devinrsmith added 3 commits November 6, 2023 11:04

f

11e3453

Collapse some hierarchies. Make batch/serial non-stateful by default

b90fa27

Review points

45d1e6f

devinrsmith force-pushed the nightly/table-service-async branch from 7775042 to 45d1e6f Compare November 6, 2023 19:05

rcaudy reviewed Nov 6, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into nightly/table-servi…

ef3e4c8

…ce-async

devinrsmith mentioned this pull request Nov 6, 2023

Immediately notify server of release when user cancels TableHandleFuture #4781

Open

Review points

09a0b13

devinrsmith requested a review from rcaudy November 6, 2023 23:52

rcaudy approved these changes Nov 7, 2023

View reviewed changes

devinrsmith merged commit 7a4119f into deephaven:main Nov 7, 2023
10 checks passed

devinrsmith deleted the nightly/table-service-async branch November 7, 2023 16:22

github-actions bot locked and limited conversation to collaborators Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add java client TableServiceAsync #4756

Add java client TableServiceAsync #4756

devinrsmith commented Nov 1, 2023

rcaudy Nov 3, 2023

devinrsmith Nov 6, 2023

rcaudy Nov 3, 2023

devinrsmith Nov 6, 2023

rcaudy Nov 3, 2023

devinrsmith Nov 5, 2023

rcaudy Nov 6, 2023

rcaudy Nov 6, 2023

devinrsmith Nov 6, 2023

devinrsmith Nov 6, 2023

rcaudy Nov 6, 2023

devinrsmith Nov 6, 2023

deephaven-internal commented Nov 7, 2023

Add java client TableServiceAsync #4756

Add java client TableServiceAsync #4756

Conversation

devinrsmith commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deephaven-internal commented Nov 7, 2023