Add JsPlugin and Jetty js-plugins #2908

devinrsmith · 2022-09-26T16:38:20Z

The JsPlugin is a server plugin that has a distribution directory, name, version, and main. main is a relative location with respect to the distribution directory that points to the main js file to load.

Jetty infrastructure support for JsPlugin is provided via HTTP js-plugins/*. (This feature is not available with Netty.) It can be explicitly disabled by setting -Dhttp.jsPlugins=false.

For more information about the JS expectations and requirements, see https://github.com/deephaven/deephaven-js-plugin-template/

Currently, there are few ways to register a JsPlugin:

via a Deephaven python plugin entry_points, see https://github.com/deephaven/deephaven-plugin
via the system property io.deephaven.server.plugin.type.JsPluginDistribution, which is a comma separated list of directories that contain package.json files.
via a Java service loader for the io.deephaven.plugin.type.JsPlugin class

devinrsmith · 2022-09-26T16:39:30Z

Depends on deephaven/deephaven-plugin#7

devinrsmith · 2022-09-26T18:15:28Z

Integration tests failing - need to bump base images after initial round of reviews. deephaven/deephaven-base-images#36

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

niloc132 · 2022-09-29T16:17:57Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+     * @throws IOException if an I/O exception occurs
+     */
+    public static JsPlugins create() throws IOException {
+        final JsPlugins jsPlugins = new JsPlugins(Files.createTempDirectory(JsPlugins.class.getName()));


We very likely don't want to use the conventional temp system - we've historically seen installations with cron jobs or the like that nuke temp space. The exception here could be if we held open FDs or something so that the files can't be deleted out from under us, but that seems wasteful. Given the expected load (likely not very high?), it might be okay to serve from the py directories directly?

If we trust the contents enough to copy them directly, we could consider just adding more Resources to the WebAppContext, and skip the extra DefaultServlet - see Resource.newResource(...) for options to build one of these that can point to a directory or even inside a zip. If we don't implicitly trust the contents of a wheel (soft links, etc, see the other remark in this file), we should only unpack or serve "real" files, and need a more complex system anyway.

Also, consider a wheel being added after startup, since python will correctly load (and i think register them). I think this works as-is today (edit: except for writing the manifest!), but possibly worth checking that case.

We can't rely on py directories - they aren't guaranteed to be actual directories. This also allows us to eventually package stuff up in java jars if we need to; not generally accessible directly via the FS apis. (The resource abstraction that python presents uses a context manager / generator that allows temporary access to a pathlib.)

Reading the jetty documentation, they strongly recommend using DefaultServlet for static resources instead of Resource directly. I'll try and find that and link.

As it stands, we don't have dynamic support for our plugin infra. It's definitely a thing we can investigate in the future, but this isn't expected to work today. #2816

It looks like even with Resource.newResource(...), everything is reference based which we don't want when dealing with python resources.

I'm not sure if there is a pre-built "serve this content from an in-memory string". Of course, we could write that servlet logic ourselves.

Being able to take advantage of DefaultServlet is nice - my comment earlier about "they strongly recommend..." was a bit incorrect, but they do say this, https://www.eclipse.org/jetty/documentation/jetty-11/programming-guide/index.html#pg-server-http-handler-use-default-servlet:

you may want to use a DefaultServlet instead of ResourceHandler

I'll continue digging to see if there is a good pre-existing solution for in-memory content.

As an alternative to temporary directory, we can use the cache directory we setup as temporary space.

Actually - org.eclipse.jetty.util.resource.PathResource#PathResource(java.nio.file.Path) seems like a solution if it actually works with non-filesystem "Path"s (ala something like https://github.com/google/jimfs). There is some hope that it "just works", given I see this in the constructor:

this.belongsToDefaultFileSystem = this.path.getFileSystem() == FileSystems.getDefault();

Reading the jetty documentation, they strongly recommend using DefaultServlet for static resources instead of Resource directly. I'll try and find that and link.

Given that DefaultServlet wraps a Resource, and we already have a DefaultServlet, I would be surprised by this. We used to only set a directory for our resource base (not to be confused with base resource), but ... that causes jetty to unpack contents to temp, and that breaks when temp gets cleaned up.

Am I incorrect that installing a new wheel at runtime will not try to call register? should we check for calling register after init and then throw or ignore it?

I've successfully used an in-memory Path (jimfs) so we don't need to deal w/ tmpfs.

Installing a wheel at runtime will not auto-register. That said, the raw plugin architecture (register(...)) does support that functionality in theory, it just needs to be plumbed through. #2816

server/jetty/src/main/java/io/deephaven/server/jetty/JettyBackedGrpcServer.java

niloc132 · 2022-09-29T21:05:11Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+                final Map<String, String> p = new LinkedHashMap<>(3);
+                p.put("name", value.name());
+                p.put("version", value.version());
+                p.put("main", value.main());


would be nice to also send the "distribution path" relative to /js-plugins/? or are we asserting that name is always the subdirectory of js-plugins, and the web client should do the string concat directly, and that names will never contain chars that aren't url-safe?

The JS side right now assumes the path is relative to js-plugins/<name>. This is the convention that @mofojed has established - but I'm fine to change it if we want.

Happy to add validation for name; are all NPM package names url safe? Note, the names do contain @ symbol and that works ok.

slashes are legal too iirc, but i dont think we want to encourage subdirectories? Also, since we want to allow non-npm modules, we might need to handle collisions, perhaps by generating dir names.

Well, the deephaven plugins do include a slash for subdirectory implications: https://www.npmjs.com/search?q=keywords%3Adeephaven-js-plugin

@deephaven/js-plugin-dashboard-object-viewer

@deephaven/js-plugin-table-example

@deephaven/js-plugin-matplotlib

@deephaven/js-plugin-plotly

I'm not sure dealing with collisions is something I'm practically concerned with right now. We could generate random prefixes (or full paths) to deal with that, but it seems like overkill ATM.

How does js plugin resolution work? For example, let's say we accidentally installed two separate versions of the js-plugin-plotly NPM package (and we "allowed" this by distinguishing them with random prefixes), how would the web UI handle this?

@mofojed

I am not concerned about multiple versions of same package right now. If we need that, can do aliasing.

The layout is similar to how node_modules is laid out right now - the subdirectory is intentional, as then all @deephaven plugins are in the same dir (scope)

niloc132 · 2022-09-30T13:22:30Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+                p.put("main", value.main());
+                pluginsList.add(p);
+            }
+            new ObjectMapper().writeValue(out, Collections.singletonMap("plugins", pluginsList));


while technically most usage of "plugins" is probably fair game, this is one that seems better to be more specific as "web content" or the like, since this isn't a complete list of plugins at all

This is the convention established by @mofojed , happy to change.

Right, but DHE had "plugins" before DHC introduced Plugin as the supertype to ObjectType, so it had meaning there already. Given that plugin != plugin, and this is intended to be a user-facing api (as we intend to encourage community development of ways to improve the platform?) we should disambiguate one way or the other.

I called out just a few cases in the patch where I think we cross a line of confusion, this being one.

This manifest.json is in a js-plugins folder already, making it clear it's js plugins. I don't think this is a problem in this case, as the context for the manifest already implies what kind of plugins they are.

plugin/src/main/java/io/deephaven/plugin/type/JsType.java

py/server/setup.py

server/jetty/src/main/java/io/deephaven/server/jetty/JettyBackedGrpcServer.java

server/src/main/java/io/deephaven/server/plugin/type/JsTypeDistribution.java

...eephaven/server/plugin/type/JsTypeDistributionFromPackageJsonSystemPropertyRegistration.java

mofojed

Looking good. Tried it out locally:

Set up jetty with a python venv (the instructions I got from Slack, the docs around this could be improved as we discussed)
Did a pip install deephaven-plugin-plotly to install the python plotly plugin
Tested with local JS plugin by adding extraJvmArgs += ['-Dio.deephaven.server.plugin.type.JsPluginDistribution=/Users/bender/dev/deephaven/oss/deephaven-js-plugins/plotly'] to server-jetty-app/build.gradle, was able to open figures
Tested after with a pip install deephaven-plugin-plotly-js to install the JS plugin via python wrapper, was able to open figures

The code snippet I used to create a figure was:

import plotly.express as px
df = px.data.iris()
fig = px.scatter(df, x="sepal_width", y="sepal_length", color="species",
                 size='petal_length', hover_data=['petal_width'])

py/server/setup.py

Tested in combination with deephaven/deephaven-core#2908; published .dev versions to PyPi for downstream consumption. Adds the python class deephaven.plugin.js.JsPlugin. This allows for the development of JS plugins which can be provided by python packages. See https://github.com/deephaven/js-plugin-template for JS expectations and requirements.

niloc132 · 2022-10-17T20:57:09Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+     */
+    public static JsPlugins create() throws IOException {
+        final FileSystem fs = Jimfs.newFileSystem(Configuration.unix());
+        final JsPlugins jsPlugins = new JsPlugins(fs.getPath("/"));


Is there ever a case where JsPlugins would be created with a different path? Would it make any sense to just pass the fs itself, or do the creation internally? The private ctor doesn't seem to add anything, new would be fewer chars, no meaningful difference (since there isnt another factory method etc).

Also, is it safe to assume that Configuration.unix() behaves well with mac and windows, no assumptions will be made that will give us a headache later?

I had another code path I was testing with earlier - a temporary local directory that was being created. If we add unit testing, we'll probably need to extend our construction techniques to better clean-up.

I'm not sure if this is a personal bias, or recommended best-practices I've seen elsewhere; but I try and keep my constructors as simple setters and simple object creation, with necessary supporting logic and initialization in static methods.

That's a good question - I wonder if we are better off choosing the in-memory representation that more closely matches the host operating system. I'll look into it a bit more, specifically wrt COPY_ATTRIBUTES

I'm not sure we want all attributes, but I'm not sure we don't, just want to be sure we decide rather than get the default.

Temp dir seems great too, though I'd hesitate about not testing the actual paths we intend to use, since jimfs by design isn't "the real thing", and those oddities might surprise us at runtime.

Thoughts about just making an on-disk zip file of the dir structure we need, and using that as a filesystem? Only one file need be created, and I believe that the zip fs impl holds open a handle, so /tmp being deleted wouldnt hurt us, and we wouldn't need to worry about having tons of and/or giant files in memory.

No objection to simple object creation, just voting for consistency of the class's own API. I tend to agree that if the logic is at all non-trivial, use a nicely named factory, but if there is a simple variant then also give it an appropriately named factory method.

I've got zip filesystem plumbed though now, I like it more than jimfs (even though jimfs is cool, it's an extra dependency).

niloc132 · 2022-10-17T21:02:22Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPluginsModule.java

+public interface JsPluginsModule {
+
+    @Provides
+    static JsPluginRegistration providesRegistration(JettyConfig config, JsPlugins plugins) {


not a singleton? in theory at least this could get hit more than once, with different JsPluginRegistrationNoOp instances? If you don't want it a singleton here, maybe make the noop into an enum or the like?

Structurally, I think this method should "delegate" to the singleton (or non-singleton) nature of the injected JsPlugins - that said, I'll improve JsPluginRegistrationNoOp as suggested.

niloc132 · 2022-10-17T21:05:13Z

server/src/main/java/io/deephaven/server/plugin/type/JsPluginDistribution.java

+        Files.walkFileTree(src, new CopyRecursiveVisitor(src, dst));
+    }
+
+    static final class PackageJson {


conventions should be documented, either in this repo or possibly in https://github.com/deephaven/js-plugin-template? esp since we hardcode the name, and fail on unknown properties (so "real" package.json files will fail it)

There is a bit of docs in io.deephaven.server.plugin.type.JsPluginDistribution#fromPackageJsonDistribution saying that name, version, and main are used from package.json. I think linking out to this repository makes sense though, will do that.

Sure, but that's in the guts of the java code - the author of a js plugin is going to look at js-plugin docs, not javadocs of a given class on the platform?

I'll add a link at JsPlugin level.

What I'm saying is that we need more than Javadoc for this - a JS/TS developer shouldn't have to already know how java/py works to be able to figure this out.

I don't expect JS/TS dev being able to necessarily decipher through this either. I hope the external js plugin documentation will be well suited for them, and then we can have docs for "here's how to convert it to PyPi package or jar for easy inclusion".

To confirm, we don't expect that a JS dev will be able to work out how to produce a jar/zip or a wheel, or what the format should be of the package.json? I don't think that's what you're saying, but it seems to be.

I'm not asking for "lots of java tutorials", but just "here's the shortest path-to-success to produce a js plugin for deephaven, what files matter, what fields matter, why your own package.json isn't valid, just a subset of it". Mostly "you can have a name, version, and main, we'll effectively ignore the version, and use the main as a relative path from your package.json file", etc. Such a plugin might have no py and no java, and no plan to add either, but wants to know the degrees of freedom they should have?

Maybe you're saying you'll file something against js-plugins to write this up? That seems like a good place, as long as it is agnostic to the language (since this should be strictly declarative)?

niloc132 · 2022-10-17T21:13:33Z

server/src/main/java/io/deephaven/server/plugin/type/JsPluginDistribution.java

+     * @return the js plugin distribution
+     * @throws IOException if an I/O exception occurs
+     */
+    public static JsPluginDistribution fromDistribution(Path distributionDir) throws IOException {


consider both ctors, or both factory methods? one of each feels weird

niloc132 · 2022-10-17T21:15:11Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+                    String.format("js plugin with name '%s' already exists", jsPlugin.name()));
+        }
+        try {
+            jsPlugin.copyTo(path.resolve(jsPlugin.name()));


if fs, then fs.getPath(jsPlugin.name())

also, consider finer-grained exceptions where they happen ("what if src doesnt exist", "what if files can't be read" etc), rather than re-wrapping exceptions on the way back out to python. Might be better to just explicitly throw that io exception to python directly?

I don't think leaning on fs.getPath makes sense in general.

The default already takes care of that:

default Path resolve(String other) { return resolve(getFileSystem().getPath(other)); }

and then one better, the implementation may override this to be even better.

I'll do some testing against files w/ bad permissions and see what the experience is like...

niloc132 · 2022-10-17T21:27:25Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }
+        plugins.put(jsPlugin.name(), jsPlugin);


maybe write this after the below IO ex doesn't happen, then merge the two try/catch blocks? We don't anticipate a reasonable state where the first succeeds, the map entry gets create, and the second fails, right?

As written, writeManifest() depends on plugins. We could modify it to writeManifest(jsPlugin), but that seems a bit ill-structured just to suit.

I'm simplifying the writeManifest a bit, but think the order makes sense as-is.

I don't expect writeManifest to fail.

Should somehow match that perspective in code then? "This can't possibly fail, so we're going to make sure the system is unusable if it somehow does manage to fail". Since you'd like the rest of the impl to be agnostic of exactly which FileSystem impl is in use, it should be expected that an ordinary write could fail for some reason. If we're actually assuming that jimfs is the only option, I'd agree with you here, but ask you make the API hard/impossible to use (even in unit tests) with a different fs.

In the current path to registrations, if the registration fails, startup will fail (explicitly made the permissions on a file bad):

2022-10-18T17:50:05.354Z | main | INFO | .d.s.p.PluginRegistration | Registering js plugin: @deephaven/js-plugin-plotly@0.1.0 / io.deephaven.server.plugin.type.JsPluginDistribution@2c413ffc Initiating shutdown due to: Uncaught exception in thread main java.lang.RuntimeException: Error in Python interpreter: Type: <class 'RuntimeError'> Value: java.nio.file.AccessDeniedException: /tmp/dh/lib64/python3.10/site-packages/deephaven_plugin_plotly_js/dist/index.js at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:90) at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111) at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116) at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:219) at java.base/java.nio.file.Files.newByteChannel(Files.java:371) at java.base/java.nio.file.Files.newByteChannel(Files.java:422) at java.base/java.nio.file.spi.FileSystemProvider.newInputStream(FileSystemProvider.java:420) at java.base/java.nio.file.Files.newInputStream(Files.java:156) at java.base/java.nio.file.CopyMoveHelper.copyToForeignTarget(CopyMoveHelper.java:125) at java.base/java.nio.file.Files.copy(Files.java:1298) at io.deephaven.server.plugin.type.JsPluginDistribution$CopyRecursiveVisitor.visitFile(JsPluginDistribution.java:141) at io.deephaven.server.plugin.type.JsPluginDistribution$CopyRecursiveVisitor.visitFile(JsPluginDistribution.java:122) at java.base/java.nio.file.Files.walkFileTree(Files.java:2725) at java.base/java.nio.file.Files.walkFileTree(Files.java:2797) at io.deephaven.server.plugin.type.JsPluginDistribution.copyRecursive(JsPluginDistribution.java:103) at io.deephaven.server.plugin.type.JsPluginDistribution.copyTo(JsPluginDistribution.java:98) at io.deephaven.server.jetty.jsplugin.JsPlugins.register(JsPlugins.java:93) at io.deephaven.server.plugin.PluginRegistrationVisitor.visit(PluginRegistrationVisitor.java:44) at io.deephaven.server.plugin.PluginRegistrationVisitor.visit(PluginRegistrationVisitor.java:17) at io.deephaven.plugin.type.JsPluginBase.walk(JsPluginBase.java:13) at io.deephaven.server.plugin.PluginRegistrationVisitor.register(PluginRegistrationVisitor.java:32) at io.deephaven.server.plugin.PluginRegistration$Counting.visit(PluginRegistration.java:73) at io.deephaven.server.plugin.PluginRegistration$Counting.visit(PluginRegistration.java:46) at io.deephaven.plugin.type.JsPluginBase.walk(JsPluginBase.java:13) at io.deephaven.server.plugin.PluginRegistration$Counting.register(PluginRegistration.java:53) at io.deephaven.server.plugin.python.CallbackAdapter.registerJsPlugin(CallbackAdapter.java:31) at org.jpy.PyLib.callAndReturnValue(Native Method) at org.jpy.PyProxyHandler.invoke(PyProxyHandler.java:120) at io.deephaven.server.plugin.python.$Proxy5.initialize_all_and_register_into(Unknown Source) at io.deephaven.server.plugin.python.PythonPluginsRegistration.registerInto(PythonPluginsRegistration.java:33) at io.deephaven.server.plugin.PluginRegistration.registerAll(PluginRegistration.java:41) at io.deephaven.server.runner.DeephavenApiServer.run(DeephavenApiServer.java:136) at io.deephaven.server.jetty.JettyMain.main(JettyMain.java:25)

niloc132 · 2022-10-17T21:28:38Z

server/src/main/java/io/deephaven/server/plugin/type/JsPluginDistribution.java

+
+        @Override
+        public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException {
+            Files.copy(dir, dst.resolve(src.relativize(dir).toString()));


why toString?

Necessary b/c path.resolve(path) requires that both paths are from the same FS. I'll add a note.

Hmm, I hit this once before, and I thought there was a kinder approach... I'll reply back if I find it.

Found it - I explicitly tested their roots, and didn't relativize if they were on a different fs - but that handle distinct filesystems of the same type, but different instances.

It does seem odd, given how Paths.get(..) is implemented, that relative Path instances aren't suitable for use on other filesystems.

niloc132 · 2022-10-17T21:31:40Z

server/src/main/java/io/deephaven/server/plugin/type/JsPluginDistribution.java

+        copyRecursive(distributionDir, destination);
+    }
+
+    private static void copyRecursive(Path src, Path dst) throws IOException {


we have commons-io elsewhere on the classpath, which has a handy FileUtils.copyDirectory that handles recursion, soft links in a oneliner - not sure if it handles all edge cases the same way, or if it can deal with jimfs, but you likely do want to handle soft links, and may want to address metadata like last-modified.

The javadocs for copy say that it follows symlinks - if you don't want that behavior, we can use NOFOLLOW_LINKS.

I'll add in COPY_ATTRIBUTES.

Yes, I'm mixing read and write - and if jimfs isn't the only possible fs impl, we need to be more careful of what could happen on writes, guard against files (or directories) already existing, not being empty, etc. Or recursively delete ahead of time?

ATM, we are guarded against overwriting since we can't register the same plugin name multiple times.

Guarded by implementation only - should probably have an explicit check for it, since the impl will happen to fail, as opposed to this not being a real use case (and indeed this is a real use case, we do intend to support it).

I don't understand Q.

A user attempting to runtime-register a plugin will see it succeed. But if they try to update it and re-register it, it will fail, and not with a "this isn't support" error, instead they'll get a "IOException: file exists" error, probably with the "absolute" path within the zip (easily confused for their own path). No amount of trying to delete their own file so that it can be created will make sense for them. Instead, if it doesn't work, and it doesn't work by design, we should give them a coherent error around that, rather than just getting lucky that it fails, and leaving them to figure out why on their own.

Well, it's also guarded by what we expose - io.deephaven.plugin.Registration.Callback is not presented to the user right now in any sort of static or easily fetch-able context. That's really what #2816 is about. So there isn't currently a way to "runtime-register" or "re-register"

Yep, no arguing that the api isn't complete to allow all possible things, but that we could provide good errors instead of bad ones for features we deliberately don't support instead of accidentally don't support. We explicitly don't support re-registering or updating plugins after they change, we can call that out with an illegal state/arg ex, instead of a "sorry that file exists" io error. We have the information to do, etc. An error when writing this zip is almost certainly "everything is broken" or "you did a thing that we don't support".

For example, without testing it, it looks like while we explicitly support plugins with a / in their name, if you attempt to register a plugin named foo which already has a file at bar/main.js and foo/bar with a file at main.js, there will be an IO error, rather than actually mapping these to different locations, or just saying "sorry some other plugin has already claimed that file name".

niloc132 · 2022-10-17T21:32:25Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+     * @return the servlet holder
+     */
+    public ServletHolder servletHolder(String name) {
+        final ServletHolder jsPlugins = new ServletHolder(name, DefaultServlet.class);


probably should explicitly handle caching, either here or in the existing caching setup, esp since that plotly plugin is not small

I'm not sure what reasonable cache values are - I'll see if we are inheriting any cache values from jetty; and at a minimum, will add in etag support.

The context itself has etags enabled, but I am not at all certain that is inherited by a sub-DefaultServlet. There are filters for caching, but they are explicit for files that include their hash in their name. Both of these are in JettyBackedGrpcServer.

Looks like we do inherit the defaults for etag. I'll add an explicit filter for NoCache.

Idle thought on caching: at least for main, we could rename the file when copying, and report the renamed file via the main value in the json. The renamed string could include a hash in it, so that the url necessarily changes when the file changes, so the file can always be cached.

Same applies for the full plugin by naming the directory of the plugin with the hash of the contents, but the directory walking has to be deterministic.

niloc132 · 2022-10-17T21:39:55Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+
+    private void writeManifest() throws IOException {
+        // note: would this be better as a custom servlet instead? (this current way is certainly easy...)
+        try (final OutputStream out = new BufferedOutputStream(Files.newOutputStream(path.resolve("manifest.json")))) {


buffering to an in-memory fs?

i ask because its going to be fully read into memory either way, and afaict never GC'd - and while the other files aren't going to be very small, this one is likely to be no bigger than a few kb?

I'm taking the FS-agnostic approach here - mentioned earlier, but this was originally written for local temporary filesystem. Even though it's not technically necessary when writing to an in-memory fs, I think it's still best practice to buffer like this.

Makes sense, I added notes in a few other places where we should be truly agnostic to follow this attitude.

niloc132 · 2022-10-18T21:28:17Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+    public static JsPlugins create() throws IOException {
+        final Path tempDir = Files.createTempDirectory(JsPlugins.class.getName());
+        tempDir.toFile().deleteOnExit();
+        final Path fsZip = tempDir.resolve("fs.zip");


more specific name, so the ownership of this file is obvious? deephaven-js-plugins.zip?

niloc132 · 2022-10-18T21:32:12Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+        fsZip.toFile().deleteOnExit();
+        // Note, the URI needs explicitly be parseable as a directory URL ending in "!/", a requirement of the jetty
+        // resource creation implementation, see
+        // org.eclipse.jetty.util.resource.Resource.newResource(java.lang.String, boolean)


probably should re/move this comment, code, since it doesn't really apply here any more, but maybe servletHolder()?

niloc132 · 2022-10-19T12:25:46Z

server/src/main/java/io/deephaven/server/plugin/python/CallbackAdapter.java

+    public void registerJsPlugin(String path, String name, String version, String main) throws IOException {
+        try {
+            callback.register(JsPluginDistribution.of(Path.of(path), name, version, main));
+        } catch (UncheckedIOException e) {


why force the wrap and unwrap, instead of just declaring the IO ex? Maybe this isnt needed now that register() declares IO ex (not sure, since there are several methods named register() that take a plugin arg...)?

So, I think the interfaces presented either need to not have checked exceptions, or to throw Exception (as opposed to IOException (io.deephaven.plugin.Registration.Callback#register) given their generic nature.

This explicit unwrap is providing slightly better stacktraces for python when something goes wrong, but isn't technically needed.

given how irritating py+java stack traces are, it might be better to have an explicit "i should be translated with only a message and a frame or three into python" java exception - the example error for permissions is daunting, esp given that java and py stack traces run in the opposite order.

not to mention if the py code that calls register might have been called by a dh app (java), which could have been started by python (dhaal)

I sympathize w/ the struggles of python + java mixed traces. Do we present java exception stack simplification anywhere else?

I worry about the added complexity and fragility of introspecting errors we "expect" to see, at least if we try to solve this on a case-by-case basis.

Yeah, this is more of a "the error should be as clean as possible, since our target audience is not python developers who have to deal with java, but js developers who have to deal with python and java, understanding neither".

What i'm asking/hoping for is to deal with expected errors as part of the "state machine" rather than leaving them to "eh, here's an IO error, figure it out"

niloc132 · 2022-10-19T12:29:50Z

server/src/main/java/io/deephaven/server/plugin/type/JsPluginDistribution.java

+        copyRecursive(distributionDir, destination);
+    }
+
+    private static void copyRecursive(Path src, Path dst) throws IOException {


Guarded by implementation only - should probably have an explicit check for it, since the impl will happen to fail, as opposed to this not being a real use case (and indeed this is a real use case, we do intend to support it).

niloc132 · 2022-10-19T12:30:48Z

server/src/main/java/io/deephaven/server/plugin/type/JsPluginDistribution.java

+        Files.walkFileTree(src, new CopyRecursiveVisitor(src, dst));
+    }
+
+    static final class PackageJson {


What I'm saying is that we need more than Javadoc for this - a JS/TS developer shouldn't have to already know how java/py works to be able to figure this out.

niloc132 · 2022-10-19T12:32:25Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+    private final URI filesystem;
+    private final List<Map<String, String>> plugins;
+
+    private JsPlugins(URI filesystem) {


you were going to add tests for this, others?

Given the test construction right now, it's easy for me to test the empty case; and at a minimum, that's testing that we correctly wrote out and read back the manifest.json file from the zip filesystem.

It probably makes sense to eventually add more HTTP tests against io.deephaven.server.jetty.JettyBackedGrpcServer that will check our expectations (etag, caching, etc), and have easier injectability for JsPlugin and other HTTP stuff we might want to test.

niloc132 · 2022-10-19T12:40:10Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+
+    @Override
+    public synchronized void register(JsPlugin jsPlugin) throws IOException {
+        for (Map<String, String> plugin : plugins) {


This structure confuses me a bit - why a map here, if there will always and only be three sets of key/value pairs? And then, why require walking the list, if the list is effectively a map from name to object (then serialize map.values() to get the actual underlying values without the extra name wrapping)?

Also, if the structure exists only for json's sake and not for any actual processing, why not use the node types that jackson provides instead of making it adapt from generic collections?

it looks like PackageJson is nearly correct for this use case, though perhaps with a name change to be clear that it is about packaging, as opposed to being a subset of the actual package.json specification.

I've created a common object called JsPluginInfo that can be both ser/deser from JSON.

niloc132 · 2022-10-19T12:40:58Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+            final Path pluginPath = fs.getPath("/", jsPlugin.name());
+            jsPlugin.copyTo(pluginPath);
+            plugins.add(Map.of(NAME, jsPlugin.name(), VERSION, jsPlugin.version(), MAIN, jsPlugin.main()));
+            writeManifest(fs);


to confirm, there is no clean way to not call this N+1 times? once when the instance is created (but only via the factory ctor), and once for each plugin added?

If we're just concerned about re-writing the manifest multiple times, we can probably come up with a solution to only write it "once".

More generally though, it's not as easy as writing a visitor that collects all the plugins and then installs them in one shot. The lifecycle concerns around io.deephaven.plugin.type.JsPlugin#copyTo for clean python interop has python opening and closing the distribution directory for consumption py/server/deephaven/server/plugin/register.py:

elif isinstance(plugin, JsPlugin): with plugin.distribution_path() as distribution_path: self._callback.registerJsPlugin(str(distribution_path), plugin.name, plugin.version, plugin.main)

Our avenues for better batch installing either involve

Adding batch registration interfaces in both python and java, plumbing as appropriate

Having an extra layer of indirection during installation, then batching up results (for example, copying everything to jimfs, and then batch to zipfs).

I'll try and document these concerns a bit in code, and see if there is an easy way to solve for just the manifest.

I did take a stab at trying to be more efficient wrt the manifest; but it wasn't very elegant and I figured it would be better to try and solve it in a more complete way outside of this PR.

#3007

niloc132 · 2022-10-19T12:42:13Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+    }
+
+    private void writeManifest(FileSystem fs) throws IOException {
+        final Path manifestPath = fs.getPath("/", MANIFEST_JSON);


using "/" violates the assumption you had expressed in chat, where this was going to be testable on a plain fs, rather than only jimfs or zip?

Correct - given that zipfs works on new filesystems and has semantics required closing the filesystem, it didn't mesh cleanly w/ how one might use jimfs or plain fs. (Note: this does not prevent JsPlugin from being local filesystem, or something else; just effects the registration implementation.)

As such, I should probably rename and document this class as working with zip filesystems explicitly. If we find we need other implementations centered aroundjimfs or localfs in the future, we can create a new JsPluginRegistration.

niloc132 · 2022-10-19T12:47:16Z

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPlugins.java

+     * @return the servlet holder
+     */
+    public ServletHolder servletHolder(String name) {
+        final ServletHolder jsPlugins = new ServletHolder(name, DefaultServlet.class);


Idle thought on caching: at least for main, we could rename the file when copying, and report the renamed file via the main value in the json. The renamed string could include a hash in it, so that the url necessarily changes when the file changes, so the file can always be cached.

Same applies for the full plugin by naming the directory of the plugin with the hash of the contents, but the directory walking has to be deterministic.

devinrsmith · 2022-10-19T23:55:53Z

Added #3005

devinrsmith · 2022-10-19T23:56:06Z

Added #3004

devinrsmith · 2022-10-20T00:23:31Z

Couple of comments I can't re-comment on (not sure why)

#2908 (comment) : I don't understand Q.

#2908 (comment) : I don't expect JS/TS dev being able to necessarily decipher through this either. I hope the external js plugin documentation will be well suited for them, and then we can have docs for "here's how to convert it to PyPi package or jar for easy inclusion".

plugin/build.gradle

niloc132 · 2022-10-24T16:59:26Z

server/src/main/java/io/deephaven/server/plugin/type/JsPluginDistribution.java

+        copyRecursive(distributionDir, destination);
+    }
+
+    private static void copyRecursive(Path src, Path dst) throws IOException {


I don't understand Q.

A user attempting to runtime-register a plugin will see it succeed. But if they try to update it and re-register it, it will fail, and not with a "this isn't support" error, instead they'll get a "IOException: file exists" error, probably with the "absolute" path within the zip (easily confused for their own path). No amount of trying to delete their own file so that it can be created will make sense for them. Instead, if it doesn't work, and it doesn't work by design, we should give them a coherent error around that, rather than just getting lucky that it fails, and leaving them to figure out why on their own.

server/jetty/src/main/java/io/deephaven/server/jetty/jsplugin/JsPluginsZipFilesystem.java

server/jetty/src/test/java/io/deephaven/server/jetty/JettyFlightRoundTripTest.java

niloc132 · 2022-10-24T18:49:54Z

server/src/main/java/io/deephaven/server/plugin/python/CallbackAdapter.java

+    public void registerJsPlugin(String path, String name, String version, String main) throws IOException {
+        try {
+            callback.register(JsPluginDistribution.of(Path.of(path), name, version, main));
+        } catch (UncheckedIOException e) {


given how irritating py+java stack traces are, it might be better to have an explicit "i should be translated with only a message and a frame or three into python" java exception - the example error for permissions is daunting, esp given that java and py stack traces run in the opposite order.

not to mention if the py code that calls register might have been called by a dh app (java), which could have been started by python (dhaal)

niloc132

Approving as-is, but I'm unhappy with the approach of making js/ts devs figure out what a java-exception-in-a-python-error is, instead of actively separating plugin semantics from filesystem semantics and dealing with the legal plugin states.

devinrsmith · 2022-12-01T16:53:07Z

Closed for now due to #3056

Add JsType plugin

c391933

devinrsmith added python DocumentationNeeded plug-ins javascript Pull requests that update Javascript code labels Sep 26, 2022

devinrsmith added this to the Sept 2022 milestone Sep 26, 2022

devinrsmith requested a review from niloc132 September 26, 2022 16:38

devinrsmith self-assigned this Sep 26, 2022

Fix tests wrt js types

33bcbd3

devinrsmith added 2 commits September 26, 2022 15:18

Move from package.json model to distribution model

e62e901

Simplify hierarchy for js type plugins - jetty gets it, netty does not

2d52e05

niloc132 reviewed Sep 30, 2022

View reviewed changes

mofojed requested changes Oct 7, 2022

View reviewed changes

devinrsmith added 4 commits October 7, 2022 12:53

Merge remote-tracking branch 'upstream/main' into js-plugin-jetty

31ea436

jimfs

cf42290

Js plugin configuration via module

2c990d6

Rename and cleanup

c491136

devinrsmith changed the title ~~Add JsType plugin~~ Add JsPlugin and Jetty js-plugins Oct 7, 2022

devinrsmith requested review from mofojed and niloc132 October 7, 2022 22:48

mofojed previously approved these changes Oct 7, 2022

View reviewed changes

py/server/setup.py Outdated Show resolved Hide resolved

devinrsmith mentioned this pull request Oct 11, 2022

Add JsPlugin deephaven/deephaven-plugin#7

Merged

devinrsmith added 2 commits October 12, 2022 15:17

Merge remote-tracking branch 'upstream/main' into js-plugin-jetty

36379b5

Bump to deephaven-plugin>=0.3.0

accb8a7

devinrsmith dismissed mofojed’s stale review via accb8a7 October 12, 2022 22:20

devinrsmith requested a review from mofojed October 13, 2022 16:47

mofojed previously approved these changes Oct 13, 2022

View reviewed changes

niloc132 reviewed Oct 17, 2022

View reviewed changes

Review responses 1 - more to come...

b9de7b6

devinrsmith dismissed mofojed’s stale review via b9de7b6 October 17, 2022 23:29

devinrsmith added 3 commits October 18, 2022 07:10

Fix inject

b3ff84f

Use zip filesystem instead of jimfs

80b54fd

add link to js repo

355e6f3

devinrsmith requested a review from niloc132 October 18, 2022 17:55

niloc132 reviewed Oct 19, 2022

View reviewed changes

devinrsmith requested a review from niloc132 October 20, 2022 00:23

Review comments

1ea6a69

devinrsmith mentioned this pull request Oct 20, 2022

Efficient registration of multiple io.deephaven.plugin.Plugin #3007

Open

niloc132 reviewed Oct 24, 2022

View reviewed changes

devinrsmith added 2 commits October 24, 2022 14:39

Merge remote-tracking branch 'upstream/main' into js-plugin-jetty

7adcbbe

Review responses

e7abb59

devinrsmith requested a review from niloc132 October 24, 2022 22:09

niloc132 previously approved these changes Oct 27, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into js-plugin-jetty

06aa44d

devinrsmith dismissed niloc132’s stale review via 06aa44d October 27, 2022 21:51

devinrsmith added 3 commits October 31, 2022 12:16

ContentType plugin generalization

0e945c1

Merge remote-tracking branch 'upstream/main' into js-plugin-jetty

fe43706

Merge remote-tracking branch 'upstream/main' into js-plugin-jetty

cbb2161

devinrsmith requested a review from niloc132 November 2, 2022 17:43

devinrsmith mentioned this pull request Nov 2, 2022

Add js-plugins pre-built support #3056

Merged

devinrsmith closed this Dec 1, 2022

github-actions bot locked and limited conversation to collaborators Dec 1, 2022

Add JsPlugin and Jetty js-plugins #2908

Add JsPlugin and Jetty js-plugins #2908

Conversation

devinrsmith commented Sep 26, 2022 • edited Loading

devinrsmith commented Sep 26, 2022

devinrsmith commented Sep 26, 2022

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

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

mofojed left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith Oct 17, 2022 • edited Loading

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

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

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

devinrsmith commented Sep 26, 2022 •

edited

Loading

devinrsmith Oct 17, 2022 •

edited

Loading