-
Notifications
You must be signed in to change notification settings - Fork 90
feat: DH-20578: Groovy remote file sourcing #7451
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.
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
base: main
Are you sure you want to change the base?
feat: DH-20578: Groovy remote file sourcing #7451
Conversation
No docs changes detected for 1cdbac4 |
a48276f to
fc106d6
Compare
777b0b4 to
4d1b51c
Compare
server/jetty-app-11/build.gradle
Outdated
| implementation project(':server-jetty-11') | ||
|
|
||
| implementation project(':extensions-flight-sql') | ||
| implementation project(':plugin-remotefilesource') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if these should have been added here since I don't see any other plugins.
server/jetty-app/build.gradle
Outdated
| implementation project(':server-jetty') | ||
|
|
||
| implementation project(':extensions-flight-sql') | ||
| implementation project(':plugin-remotefilesource') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if these should have been added here since I don't see any other plugins.
3d7a03e to
f0ced57
Compare
f0ced57 to
b70f606
Compare
...otefilesource/src/main/java/io.deephaven.remotefilesource/RemoteFileSourceMessageStream.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Utility methods for working with protobuf messages in the client-side JavaScript environment. | ||
| */ | ||
| public class JsProtobufUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...how sure are we that we actually need any/all of this? doesnt the generated message types give us this for free?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we needed it in order to support the Any wrapper?
| @JsType(namespace = "dh.remotefilesource", name = "RemoteFileSourceService") | ||
| public class JsRemoteFileSourceService extends HasEventHandling { | ||
| /** Event name for generic messages from the server */ | ||
| @JsProperty(namespace = "dh.remotefilesource.RemoteFileSourceService") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this annotation shouldnt be necessary, should get it from the type's namespace+name, ditto below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got warnings for the @link #EVENT_MESSAGE in the header comment. Claude suggested this fix which worked. Is there a better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By adding hte JsType annotation at the class top level, all public static constants are added to the JS automatically. See JsPartitionedTable for some precedent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you seeing this warning? I'm not seeing any warning in the header or anywhere else in the file related to this when I remove this annotation (the @JsProperty annotation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mofojed
If I remove the annotation and build the JS API types via ./gradlew :web-client-api:types:assemble
I get:
[warning] Failed to resolve link to "dh.remotefilesource.EVENT_REQUEST_SOURCE" in comment for dh.remotefilesource.RemoteFileSourceService.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, there are a number of other warnings as well. I just didn't want to keep adding to them:
[warning] Failed to resolve link to "dh.convertArrayToString" in comment for dh.PreviewOptions.
[warning] Failed to resolve link to "dh.array" in comment for dh.PreviewOptions.
[warning] Failed to resolve link to "dh.transportFactory" in comment for dh.ConnectOptions.useWebsockets.
[warning] Failed to resolve link to "dh.useWebsockets" in comment for dh.ConnectOptions.transportFactory.
[warning] Failed to resolve link to "Table.uncoalesced" in comment for dh.Column.isPartitionColumn.
[warning] Failed to resolve link to "Promise" in comment for dh.IdeConnection.mergeTables.
[warning] Failed to resolve link to "io.deephaven.web.shared.fu.JsRunnable" in comment for dh.IdeConnection.onLogMessage.
[warning] Failed to resolve link to "io.deephaven.web.shared.fu.JsRunnable" in comment for dh.IdeConnection.onLogMessage.__type.
[warning] Failed to resolve link to "dh.includeConstituents" in comment for dh.TreeTable.
[warning] Failed to resolve link to "TreeRowImpl.hasChildren" in comment for dh.TreeTable.
[warning] Failed to resolve link to "java.lang.UnsupportedOperationException" in comment for dh.TreeTable.applyCustomColumns.
[warning] Failed to resolve link to "io.deephaven.engine.table.PartitionedTable.merge" in comment for dh.PartitionedTable.getMergedTable.
[warning] Failed to resolve link to "DataOptions.SubscriptionOptions" in comment for dh.Table.subscribe.
[warning] Failed to resolve link to "io.deephaven.web.client.api.subscription.AbstractTableSubscription.close" in comment for dh.Table.createSubscription.
[warning] Failed to resolve link to "Table.uncoalesced" in comment for dh.Table.size.
[warning] Failed to resolve link to "Promise" in comment for dh.IdeSession.getTreeTable.
[warning] Failed to resolve link to "Promise" in comment for dh.IdeSession.mergeTables.
[warning] Failed to resolve link to "Promise" in comment for dh.IdeSession.emptyTable.
[warning] Failed to resolve link to "Promise" in comment for dh.IdeSession.timeTable.
[warning] Failed to resolve link to "JsTable#createSnapshot(Object)" in comment for dh.TableViewportSubscription.snapshot.
[warning] Failed to resolve link to "dh.remotefilesource.EVENT_REQUEST_SOURCE" in comment for dh.remotefilesource.RemoteFileSourceService.
...pi/src/main/java/io/deephaven/web/client/api/remotefilesource/JsRemoteFileSourceService.java
Outdated
Show resolved
Hide resolved
...pi/src/main/java/io/deephaven/web/client/api/remotefilesource/JsRemoteFileSourceService.java
Show resolved
Hide resolved
...otefilesource/src/main/java/io.deephaven.remotefilesource/RemoteFileSourceMessageStream.java
Show resolved
Hide resolved
...otefilesource/src/main/java/io.deephaven.remotefilesource/RemoteFileSourceMessageStream.java
Outdated
Show resolved
Hide resolved
...otefilesource/src/main/java/io.deephaven.remotefilesource/RemoteFileSourceMessageStream.java
Outdated
Show resolved
Hide resolved
plugin/src/main/java/io/deephaven/plugin/type/PluginMarker.java
Outdated
Show resolved
Hide resolved
| "RemoteFileSourcePluginFetchRequest must contain a valid result_id"); | ||
| } | ||
|
|
||
| final String pluginName = request.getPluginName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it appears that DeephavenRemoteFileSourcePlugin is always the pluginName set by the client, so we end up with exactly one PluginMarker. Do we anticipate more of these, or specific clients who can control this? Nothing seems able to clear old instances, do we want a mechanism (liveness?) for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I anticipate that the Python version of the plugin may make use of this at some point.
Clearing instances is probably a good idea. Any suggestions on best way to do that?
mofojed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality seems good overall, nice.
engine/base/src/main/java/io/deephaven/engine/util/RemoteFileSourceClassLoader.java
Outdated
Show resolved
Hide resolved
| throw new IllegalArgumentException("messageStream must not be null"); | ||
| } | ||
|
|
||
| executionContext = new RemoteFileSourceExecutionContext(messageStream, resourcePaths); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we throw if the executionContext is already set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. executionContext is static and only 1 MessageStream instance can claim it at a time. This probably doesn't impact Core+, but in Community it could matter if you had multiple connections to the worker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a better block comment to the method.
...otefilesource/src/main/java/io.deephaven.remotefilesource/RemoteFileSourceMessageStream.java
Outdated
Show resolved
Hide resolved
...otefilesource/src/main/java/io.deephaven.remotefilesource/RemoteFileSourceMessageStream.java
Outdated
Show resolved
Hide resolved
...otefilesource/src/main/java/io.deephaven.remotefilesource/RemoteFileSourceMessageStream.java
Outdated
Show resolved
Hide resolved
| @JsType(namespace = "dh.remotefilesource", name = "RemoteFileSourceService") | ||
| public class JsRemoteFileSourceService extends HasEventHandling { | ||
| /** Event name for generic messages from the server */ | ||
| @JsProperty(namespace = "dh.remotefilesource.RemoteFileSourceService") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By adding hte JsType annotation at the class top level, all public static constants are added to the JS automatically. See JsPartitionedTable for some precedent.
| DomGlobal.setTimeout(ignore -> fireEvent(EVENT_REQUEST_SOURCE, | ||
| new ResourceRequestEvent(message.getRequestId(), request)), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment why this is adding a timeout here instead of just calling it right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually 100% sure why this is needed except that I saw precedence in JsWidget for dispatching new events in response to message stream events. I assume it's desirable to ensure this happens on the next tick, but I can't speak to a specific problem it may cause if we don't do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the history in JsWidget, looks like it was added with this PR: #6715
In that case, I think the issue was the promise for fetching the widget would resolve after we had already fired off the events, so client didn't have time to add a listener to actually receive the event. So just put emitting the event on the next tick after resolving.
I guess in theory that might be possible here as well... though maybe we should be checking if there's no listeners and throwing in that case, or force a listener to be passed in when retrieving the plugin. When there's a request from the server, it's blocked until there's a response, yeah? So we must have a listener to respond here (or we should default to a no-op response, but then why bother fetching the plugin...), and it doesn't make sense to have multiple listeners registered (since multiple responses for one request is invalid), so really we want exactly one listener... may as well enforce it with creation of the service?
In CoreClient#getRemoteFileSourceService(), we're returning a new instance of the service plugin whenever that method is called - what happens if there's multiple instances? We really only want one instance, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made 2 primary changes to address this:
CoreClient#getRemoteFileSourceService()now creates a single instance of JsRemoteFileSourceService- JsRemoteFileSourceService enforces that only 1
EVENT_REQUEST_SOURCEhandler can be registered. It also throws if a request for source is received and there are no registered event handlers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also removed the setTimeout
...pi/src/main/java/io/deephaven/web/client/api/remotefilesource/JsRemoteFileSourceService.java
Outdated
Show resolved
Hide resolved
mofojed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, had some questions about fetching an instance of the service and how we handle multiple instances.
DH-20578: This PR implements a RemoteFileSource plugin in Deephaven Core that can request file sources from a remote client. Key points of interest
Related PRs
Testing
Community
:web-client-api:gwtCompile server-jetty-app:run -Panonymous -Pgroovy)Workspace/Groovy/groovy-remote-docs-demofolder underREMOTE IMPORT SOURCESand click the + button to add test folder.Core+
Changes are deployed to
bmingles-remote-file-sourcevm. Can also configure VS Code to connect to https://bmingles-remote-file-source.int.illumon.com:8123/ and run same code