Support jetty as a server for deephaven #1731

niloc132 · 2021-12-22T19:28:01Z

The default gRPC-java server implementation is backed by Netty, which has some limitations for us, and has required us to distribute the application as a set of docker containers. This patch adds support for running the server inside of Jetty, a java servlet container, and also bundles in the static html/js/css required for the JS API and UI, as well as a websocket handler that can allow the browser to connect without full grpc support.

Fixes #1270

build.gradle

log-factory/sources/slf4j-to-log/src/main/java/org/slf4j/impl/StaticLoggerBinder.java

...plane-grpc-flight/src/main/java/io/deephaven/proto/flight/util/FlightExportTicketHelper.java

server/build.gradle

server/jetty/README.md

server/jetty/src/main/resources/jetty-logging.properties

server/jetty/src/main/resources/logging.properties

server/src/test/app.d/02-python.py

server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java

web/client-api/src/main/java/io/deephaven/web/public/table_viewport.html

devinrsmith

Good stuff. Some commented out stuff that we should probably cleanup?

Running locally, I see that server-jetty is serving up a META-INF listing from http://localhost:10000. Should we filter to only include ide/ and jsapi/?

I'm guessing it was this way before, but some of our "Feature Demos" in jsapi/ seem to be dependent on python? Might be good to generalize and use non-console apis? (I think this is the same as your comment on table_viewport file).

Should we auto-redirect to ide/?

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

build.gradle

niloc132 · 2021-12-27T19:19:28Z

Should we auto-redirect to ide/?

Fixed.

devinrsmith

I'll check it out locally soon and go through some QA.

buildSrc/src/main/groovy/Classpaths.groovy

devinrsmith · 2021-12-27T22:59:36Z

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

+ // set expiry to one year in the future
+ final Calendar calendar = Calendar.getInstance();
+ calendar.setTime(new Date());
+ calendar.add(Calendar.YEAR, 1);
+ httpResponse.setDateHeader("Expires", calendar.getTime().getTime());
+ // Note: immutable tells firefox to never revalidate as data will never change
+ httpResponse.setHeader("Cache-control", "max-age=" + YEAR_IN_SECONDS + ", public, immutable");
+ httpResponse.setHeader("Pragma", "");
+
+ filterChain.doFilter(request, response);


I'd not a web-caching expert, but does this potentially (incorrectly) prevent the browser for getting updated resources after the server has been updated?

Are etags plumbed through?

Take a look at where we plug in the CacheFilter over in JettyBackedGrpcServer - those files in the static dir are all generated with their hash as part of their name, so that without a name change, the contents are (in theory) unchanged.

These filters and their configuration are the same as in DHE - to a fault, I didn't correct the path to be /ide/...

devinrsmith · 2021-12-27T23:04:52Z

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

+ // Always add eTags
+ context.setInitParameter("org.eclipse.jetty.servlet.Default.etags", "true");


Ah, here are the etags. Still interested in if we are over-caching with our cache contrrol directive.

server/src/test/app.d/02-python.py

server/jetty/README.md

devinrsmith · 2021-12-27T23:32:21Z

server/jetty/README.md

+
+See https://github.com/deephaven/deephaven-core/issues/1657 for more discussion
+
+1. On MacOS there is an extra patch to apply at this time, to add an extra argument to clang,


On Fedora, I also needed python development headers installed. That looked something like

sudo dnf install python-devel

for me. Likely want a note about headers, and how to get them for ubuntu, fedora, maybe others?

Probably, but better put it #1657 if we require that, this isn't meant to be exhaustive at this time.

devinrsmith · 2021-12-27T23:34:03Z

server/jetty/README.md

+
+ $ cd py/jpy
+ $ JAVA_HOME=/path/to/your/java/home # Customize this to fit your computer
+ $ python setup.py bdist_wheel


Unfortunately, this is breaking for me (Fedora 35 running on python 3.10.1), so I'm not able to test locally.

src/main/c/jpy_jobj.c: In function ‘JType_InitSlots’: src/main/c/jpy_jobj.c:714:24: error: lvalue required as left operand of assignment 714 | Py_REFCNT(typeObj) = 1;

Probably time to incorporate jpy-consortium/jpy#14?

(Not a merge blocker, but should likely try to follow up sooner rather than later.)

We don't explicitly support any python version that we don't package at this time (and probably we should actually test the matrix of packages we ship before getting into testing cross OS, py vers, arch...)

We only "package" python 3.7, right? In your example though, it looks like you are building w/ 3.9?

It would be nice to get back into more native packaging / releasing of artifacts as wheels with each release; in which case we could be very explicit about what we expect should work.

We only package 3.7 yes, and it happens to work with 3.9 too as far as I can tell. pyenv probably should be used for now to try out a different version.

devinrsmith

Looking good. We know about the follow up issues. I've done some light QA of w2w across a matrix of different server types.

niloc132 added the build label Dec 22, 2021

niloc132 added this to the Dec 2021 milestone Dec 22, 2021

niloc132 requested a review from devinrsmith December 22, 2021 19:28

niloc132 added the DocumentationNeeded label Dec 22, 2021

niloc132 commented Dec 22, 2021

View reviewed changes

devinrsmith reviewed Dec 23, 2021

View reviewed changes

niloc132 force-pushed the 1270-jetty-server branch from 0fc7010 to 02db307 Compare December 23, 2021 17:31

niloc132 force-pushed the 1270-jetty-server branch from 48d4960 to 7c666ee Compare December 27, 2021 22:41

devinrsmith reviewed Dec 27, 2021

View reviewed changes

niloc132 marked this pull request as ready for review December 27, 2021 23:15

devinrsmith reviewed Dec 27, 2021

View reviewed changes

server/jetty/README.md Outdated Show resolved Hide resolved

devinrsmith reviewed Dec 27, 2021

View reviewed changes

server/jetty/README.md Show resolved Hide resolved

devinrsmith reviewed Dec 27, 2021

View reviewed changes

niloc132 mentioned this pull request Dec 28, 2021

EPIC: Jetty follow-up tracking issue #1744

Closed

niloc132 added 14 commits December 28, 2021 08:46

Move plot->grpc serializer. This will be rewritten soon with custom objs

eaeab74

Move server to server - TODO fix server deps

4393545

Move roundtripflighttest to be reusable between netty/jetty

7557df1

Mark package issues for followup

dc6da84

server squish

0f2d068

make start static method an instance methods - fix deps

afaa399

rearrange and other slush

0c1938f

Move server tests

99692e8

spotless

08e9315

Buildable checkpoint, server replaces grpc-api

7ea0cee

Checkpoint WIP with working jetty+ui

5d02168

rebase damage, add grpc-servlet

d83f420

more rebase damage

ff04f06

clarify dependencies further, reduce code repitition

616f1dd

niloc132 added 19 commits December 28, 2021 08:46

format grpc to match dh

9195f28

websocket cleanup

7d05549

cleanup for merge

f0cf180

Update jetty/netty start scripts and artifacts

ac94b00

remove leftovers of grpc-api, add jetty readme

8b4b945

cleanup before merge

bb60958

fix grpc_api package

a70c017

Actually run flight round trip tests, but not yet with right server

6b21638

self-review cleanup

dd5679a

hack and slash to get netty to manage its own server

8fe039b

Clean up shutdown and reenable flight round trip test for each backend

2530706

spotless, unchecked ioex

4f29b01

Added caching/redirect filters, updated samples to support groovy/py

e56c8a6

Update slf4j to use serviceloader

739e40c

spotless

014dbdb

cleanup for review

a5185a4

Improve readme for server-jetty, with instructions for python

1c8aa07

local cached codegen issue

a96c67e

review feedback

86b6dd3

niloc132 force-pushed the 1270-jetty-server branch from c09258c to 86b6dd3 Compare December 28, 2021 14:47

devinrsmith self-requested a review December 28, 2021 18:09

devinrsmith approved these changes Dec 28, 2021

View reviewed changes

niloc132 merged commit e90b74c into deephaven:main Dec 28, 2021

github-actions bot locked and limited conversation to collaborators Dec 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support jetty as a server for deephaven #1731

Support jetty as a server for deephaven #1731

niloc132 commented Dec 22, 2021

devinrsmith left a comment

niloc132 commented Dec 27, 2021

devinrsmith left a comment

devinrsmith Dec 27, 2021

niloc132 Dec 27, 2021

devinrsmith Dec 27, 2021

devinrsmith Dec 27, 2021

niloc132 Dec 27, 2021

devinrsmith Dec 27, 2021 •

edited

Loading

niloc132 Dec 27, 2021

devinrsmith Dec 27, 2021

niloc132 Dec 28, 2021

devinrsmith left a comment

		// Always add eTags
		context.setInitParameter("org.eclipse.jetty.servlet.Default.etags", "true");


		See https://github.com/deephaven/deephaven-core/issues/1657 for more discussion

		1. On MacOS there is an extra patch to apply at this time, to add an extra argument to clang,

Support jetty as a server for deephaven #1731

Support jetty as a server for deephaven #1731

Conversation

niloc132 commented Dec 22, 2021

devinrsmith left a comment

Choose a reason for hiding this comment

niloc132 commented Dec 27, 2021

devinrsmith 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

devinrsmith Dec 27, 2021 • 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

devinrsmith left a comment

Choose a reason for hiding this comment

devinrsmith Dec 27, 2021 •

edited

Loading