-
Notifications
You must be signed in to change notification settings - Fork 84
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
Application mode for long running workers #1082
Application mode for long running workers #1082
Conversation
ad825c2
to
9f591f4
Compare
application-mode/src/main/java/io/deephaven/db/appmode/Application.java
Outdated
Show resolved
Hide resolved
application-mode/src/main/java/io/deephaven/db/appmode/Application.java
Outdated
Show resolved
Hide resolved
application-mode/src/main/java/io/deephaven/db/appmode/ApplicationConfig.java
Outdated
Show resolved
Hide resolved
application-mode/src/main/java/io/deephaven/db/appmode/Field.java
Outdated
Show resolved
Hide resolved
application-mode/src/main/java/io/deephaven/db/appmode/ApplicationUtil.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/app_mode/ApplicationServiceGrpcImpl.java
Outdated
Show resolved
Hide resolved
grpc-api/src/test/java/io/deephaven/db/appmode/ApplicationConfigTest.java
Outdated
Show resolved
Hide resolved
application-mode/src/main/java/io/deephaven/db/appmode/ApplicationState.java
Outdated
Show resolved
Hide resolved
DB/src/main/java/io/deephaven/db/util/DelegatedScriptSession.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/ide/shared/IdeSession.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java
Show resolved
Hide resolved
f14706b
to
0c49b53
Compare
web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableChanges.java
Show resolved
Hide resolved
proto/proto-backplane-grpc/src/main/proto/deephaven/proto/application.proto
Outdated
Show resolved
Hide resolved
/* | ||
* A lightweight object describing the exposed field. | ||
*/ | ||
message FieldInfo { |
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 any chance you want an icon field here? easy to add later, but to differentiate different plots, different "custom" things.
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, why isn't this a table? wouldnt it make future filtering easier, sorting, eventual access control...
@@ -0,0 +1,7 @@ | |||
import io.deephaven.appmode.ApplicationContext | |||
import io.deephaven.db.tables.utils.TableTools |
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 should be unnecessary, right? or are we not doing "free" imports any more?
probably should write the kind of code we expect to have users write.
--
also, would it not make sense to have the AppContext import and app = AppContext.get() binding already done for you for these scripts? since any script that does anything interesting will need to do it anyway..
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'd like to get rid of "free" imports here where we have the chance to do something.
Also, I don't like implicit bindings that scripts just "know" about. Often times, I'd see PQs at customers where they would do:
db = (Database)db
just so they would have strong typing. I'd prefer to create a pattern where scripts are explicit.
Alternatively, we can have a pattern where instead of a "script" the scripts provide a function with a known signature:
def script_eval(app):
...
But that feels a bit clunkier.
grpc-api/src/main/java/io/deephaven/appmode/ApplicationContext.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/appmode/ApplicationFactory.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/app_mode/ApplicationTicketResolver.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/app_mode/ApplicationTicketResolver.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/console/ConsoleServiceGrpcImpl.java
Show resolved
Hide resolved
495e61c
to
ace7de6
Compare
caff216
to
8149f0c
Compare
diff.created.put(name, toType); | ||
} else if (toValue == null) { | ||
diff.removed.put(name, fromType); | ||
} else if (fromType != toType) { |
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.
is exact equality desired? or .equals(...)
?
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.
Hmm. I prefer exact equality. We're trying to detect changes that have occurred via black-box checking around the two sets of existing items in the query scope. If you swap from instance A to instance B even if A.equals(B) I think there is value to propagate a modification.
With deep equality it's not too hard to fabricate an example that will chew up lots of CPU time checking for equality. For example, an immutable linked list that gets modified only near the tail would yield terrible performance.
} | ||
} | ||
interface Listener { | ||
void onScopeChanges(ScriptSession scriptSession, Changes changes); |
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.
Do we think explicitly passing ScriptSession is necessary? I might imagine that the entity creating the listener will have the listener scoped appropriately.
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 have one listener injected via Dagger. Not one per script session. Let's ignore the single global script session in the context of this API, please.
void onNewField(ApplicationState app, Field<?> field); | ||
|
||
void onRemoveField(ApplicationState app, String name); |
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.
Do the listeners need to be passed the state?
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.
The listener is provided to the application state. Yes; I want to be able to dynamically update the application state and have it propagate through the system.
Am I misunderstanding the question?
application-mode/src/main/java/io/deephaven/appmode/ApplicationState.java
Outdated
Show resolved
Hide resolved
application-mode/src/main/java/io/deephaven/appmode/ApplicationUtil.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/appmode/ApplicationServiceGrpcImpl.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/appmode/ApplicationServiceGrpcImpl.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/appmode/ApplicationServiceGrpcImpl.java
Outdated
Show resolved
Hide resolved
import java.util.concurrent.locks.ReentrantLock; | ||
|
||
@Singleton | ||
public class ApplicationServiceGrpcImpl extends ApplicationServiceGrpc.ApplicationServiceImplBase |
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 convinced we are handling all edge cases w/ this impl without more review time - continuing on for now.
4c2c5ba
to
b783207
Compare
1aaa5a3
to
9c11ec6
Compare
9f9ca26
to
634c47a
Compare
634c47a
to
29e5965
Compare
New Flags:
Due to the restriction that we have no more than one deephaven script session, python and groovy applications cannot be mixed at this time. Note that multiple python (or groovy) scripts share the same query scope which bleed into each other, and into the query scope available to a REPL user. The
deephaven.console.disabled
flag is handy here, but the user is also welcome to encapsulate any state they wish does not leak to the REPL. The variety of options here yield a ton of flexibility.Supportive Functionality:
Bug Fixes:
Sample Groovy Application
File:
/data/app.d/01-groovy.app
File:
/data/app.d/01-groovy.groovy
Description: use ApplicationContext and lambdas to encapsulate your application state
File:
/data/app.d/01-groovy-qs.groovy
Description: you can leak content via the query scope
Sample Static Class Application
File:
/data/app.d/00-static.app
Class Impl:
Note that the DynamicApplication can add/remove fields during the runtime of the application, and is just as easy an API to use.