ConfigService gRPC and JS API #2979
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package io.deephaven.extensions.barrage; | ||
|
|
||
| import io.deephaven.barrage.flatbuf.BarrageMessageWrapper; | ||
|
|
||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.UncheckedIOException; | ||
| import java.util.Properties; | ||
|
|
||
| /** | ||
| * Resolves the runtime version of Barrage, in case it has been updated on the classpath beyond what this project was | ||
| * built with. Should be invoked before it will first be requested to ensure it is set properly. | ||
| */ | ||
| public class VersionLookup { | ||
| public static void lookup() { | ||
| String version; | ||
|
|
||
| try (InputStream is = BarrageMessageWrapper.class | ||
| .getResourceAsStream("/META-INF/maven/io.deephaven.barrage/barrage-format/pom.properties")) { | ||
| Properties properties = new Properties(); | ||
| properties.load(is); | ||
| version = properties.getProperty("version"); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException("Failed to load barrage version, is it on the classpath?", e); | ||
| } | ||
| System.setProperty("barrage.version", version); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| /* | ||
| * Copyright (c) 2016-2022 Deephaven Data Labs and Patent Pending | ||
| */ | ||
| syntax = "proto3"; | ||
|
|
||
| package io.deephaven.proto.backplane.grpc; | ||
|
|
||
| option java_multiple_files = true; | ||
| option optimize_for = SPEED; | ||
| option go_package = "github.com/deephaven/deephaven-core/go/internal/proto/config"; | ||
|
|
||
| /** | ||
| * Provides simple configuration data to users. Unauthenticated users may call GetAuthenticationConstants | ||
| * to discover hints on how they should proceed with providing their identity, while already-authenticated | ||
| * clients may call GetConfigurationConstants for details on using the platform. | ||
| */ | ||
| service ConfigService { | ||
| rpc GetAuthenticationConstants(AuthenticationConstantsRequest) returns (AuthenticationConstantsResponse) {} | ||
| rpc GetConfigurationConstants(ConfigurationConstantsRequest) returns (ConfigurationConstantsResponse) {} | ||
|
Comment on lines
+18
to
+19
There was a problem hiding this comment. I'm wondering about the "Constants" part of it - at least based on the current impl, users could manually set and change these values at runtime... There was a problem hiding this comment. Yep, naming is here to be critiqued. I don't think we want the server to push changes to these configuration values, so from a given client's perspective, we should assume that config values received are functionally constants, but at the same time, there is nothing preventing the server from changing them at runtime without a restart. There was a problem hiding this comment. On DHE the config values are certainly treated as constants. I'm alright if they change at runtime as long as there's a notification there's been a change, such that UI can re-load appropriately. It's definitely less complex if we just treat them as constants. There was a problem hiding this comment. If we do have changes, sending a disconnect also would force the client to reconnect/fetch the server config values again as well. I don't think we're expecting any use cases where config values change on the fly, I'd think it's easier to just do disconnect/reload everything if the user does want to change config values (which shouldn't happen often?) |
||
| } | ||
|
|
||
| message AuthenticationConstantsRequest {} | ||
| message ConfigurationConstantsRequest {} | ||
| message AuthenticationConstantsResponse { | ||
| repeated ConfigPair config_values = 1; | ||
| } | ||
| message ConfigurationConstantsResponse { | ||
| repeated ConfigPair config_values = 1; | ||
| } | ||
| message ConfigPair { | ||
| string key = 1; | ||
| oneof value {//leave room for more types | ||
| string string_value = 2; | ||
niloc132 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
There was a problem hiding this comment. So, I do want to talk about pros and cons of our choices here wrt https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/struct.proto. Is there benefit to relying on Struct/Value instead? Or, we could potentially mirror it with our own type and leave out the parts we don't need right now: message Value {
// The kind of value.
oneof kind {
// Represents a string value.
string string_value = 3;
}
}This might leave us room in the future if we did want to transition to upstream types. There was a problem hiding this comment. Agreed, in theory, will make the change. I'm not sure what you mean by stronger typing though, the key is still a string the value is still whatever fits in the one-of/etc, which is still checked by tag. Indeed, the
Functionally, the only difference here is that the oneof is moved out to its own message type, so we just pay for an extra tag "go decode a value now" "here's the type of the data" instead of just "here's the type of the data". There was a problem hiding this comment. Ah, great link. I didn't realize the semantics of map were equivalent to repeated. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| package io.deephaven.server.config; | ||
|
|
||
| import io.deephaven.configuration.Configuration; | ||
| import io.deephaven.extensions.barrage.util.GrpcUtil; | ||
| import io.deephaven.internal.log.LoggerFactory; | ||
| import io.deephaven.io.logger.Logger; | ||
| import io.deephaven.proto.backplane.grpc.AuthenticationConstantsRequest; | ||
| import io.deephaven.proto.backplane.grpc.AuthenticationConstantsResponse; | ||
| import io.deephaven.proto.backplane.grpc.ConfigPair; | ||
| import io.deephaven.proto.backplane.grpc.ConfigServiceGrpc; | ||
| import io.deephaven.proto.backplane.grpc.ConfigurationConstantsRequest; | ||
| import io.deephaven.proto.backplane.grpc.ConfigurationConstantsResponse; | ||
| import io.grpc.stub.StreamObserver; | ||
|
|
||
| import javax.inject.Inject; | ||
| import java.util.Arrays; | ||
| import java.util.Objects; | ||
| import java.util.function.Consumer; | ||
|
|
||
| /** | ||
| * Serves specified configuration properties to gRPC clients. | ||
| */ | ||
| public class ConfigServiceGrpcImpl extends ConfigServiceGrpc.ConfigServiceImplBase { | ||
| private static final Logger log = LoggerFactory.getLogger(ConfigServiceGrpcImpl.class); | ||
| private static final String VERSION_LIST_PROPERTY = "client.version.list"; | ||
|
|
||
| private static final String AUTH_CLIENT_CONFIG_PROPERTY = "authentication.client.configuration.list"; | ||
|
|
||
| // TODO consider a mechanism for roles for these | ||
| private static final String CLIENT_CONFIG_PROPERTY = "client.configuration.list"; | ||
|
|
||
| private final Configuration configuration = Configuration.getInstance(); | ||
|
|
||
| @Inject | ||
| public ConfigServiceGrpcImpl() { | ||
| // On startup, lookup the versions to make available. | ||
| for (String pair : configuration.getStringArrayFromProperty(VERSION_LIST_PROPERTY)) { | ||
| pair = pair.trim(); | ||
| if (pair.isEmpty()) { | ||
| continue; | ||
| } | ||
| String[] split = pair.split("="); | ||
| if (split.length != 2) { | ||
| throw new IllegalArgumentException("Missing '=' in " + VERSION_LIST_PROPERTY); | ||
| } | ||
| String key = split[0] + ".version"; | ||
| if (configuration.hasProperty(key)) { | ||
| throw new IllegalArgumentException("Configuration already has a key for '" + key + "'"); | ||
| } | ||
| String className = split[1]; | ||
| try { | ||
| configuration.setProperty(key, Class.forName(className, false, getClass().getClassLoader()).getPackage() | ||
| .getImplementationVersion()); | ||
|
There was a problem hiding this comment. Very neat, I didn't know this existed. |
||
| } catch (ClassNotFoundException e) { | ||
| throw new IllegalArgumentException("Failed to find class to get its version '" + className + "'"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void getAuthenticationConstants(AuthenticationConstantsRequest request, | ||
| StreamObserver<AuthenticationConstantsResponse> responseObserver) { | ||
| GrpcUtil.rpcWrapper(log, responseObserver, () -> { | ||
| AuthenticationConstantsResponse.Builder builder = AuthenticationConstantsResponse.newBuilder(); | ||
| collectConfigs(builder::addConfigValues, AUTH_CLIENT_CONFIG_PROPERTY); | ||
| responseObserver.onNext(builder.build()); | ||
| responseObserver.onCompleted(); | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public void getConfigurationConstants(ConfigurationConstantsRequest request, | ||
| StreamObserver<ConfigurationConstantsResponse> responseObserver) { | ||
| GrpcUtil.rpcWrapper(log, responseObserver, () -> { | ||
| ConfigurationConstantsResponse.Builder builder = ConfigurationConstantsResponse.newBuilder(); | ||
| collectConfigs(builder::addConfigValues, CLIENT_CONFIG_PROPERTY); | ||
| responseObserver.onNext(builder.build()); | ||
| responseObserver.onCompleted(); | ||
| }); | ||
| } | ||
|
|
||
| private void collectConfigs(Consumer<ConfigPair> addConfigValues, String clientConfigProperty) { | ||
| Arrays.stream(configuration.getStringWithDefault(clientConfigProperty, "").split(",")) | ||
| .map(String::trim) | ||
| .filter(key -> !key.isEmpty()) | ||
| .map(key -> { | ||
| String value = configuration.getStringWithDefault(key, null); | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| return ConfigPair.newBuilder() | ||
| .setKey(key) | ||
| .setStringValue(value) | ||
| .build(); | ||
| }) | ||
| .filter(Objects::nonNull) | ||
| .forEach(addConfigValues); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package io.deephaven.server.config; | ||
|
|
||
| import dagger.Binds; | ||
| import dagger.Module; | ||
| import dagger.multibindings.IntoSet; | ||
| import io.grpc.BindableService; | ||
|
|
||
| @Module | ||
| public interface ConfigServiceModule { | ||
| @Binds | ||
| @IntoSet | ||
| BindableService bindConfigServiceGrpcImpl(ConfigServiceGrpcImpl instance); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,19 @@ | ||
| package io.deephaven.web.client.api; | ||
|
|
||
| import elemental2.core.JsArray; | ||
| import elemental2.promise.Promise; | ||
| import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb.AuthenticationConstantsRequest; | ||
| import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb.AuthenticationConstantsResponse; | ||
| import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb.ConfigPair; | ||
| import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb.ConfigurationConstantsRequest; | ||
| import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb.ConfigurationConstantsResponse; | ||
| import io.deephaven.web.client.api.storage.JsStorageService; | ||
| import io.deephaven.web.shared.fu.JsBiConsumer; | ||
| import io.deephaven.web.shared.fu.JsFunction; | ||
| import jsinterop.annotations.JsType; | ||
|
|
||
| import java.util.function.Consumer; | ||
|
|
||
| @JsType(namespace = "dh") | ||
| public class CoreClient extends QueryConnectable<CoreClient> { | ||
| public static final String EVENT_CONNECT = "connect", | ||
|
|
@@ -26,6 +36,18 @@ public CoreClient(String serverUrl) { | |
| this.serverUrl = serverUrl; | ||
| } | ||
|
|
||
| private <R> Promise<String[][]> getConfigs(Consumer<JsBiConsumer<Object, R>> rpcCall, | ||
| JsFunction<R, JsArray<ConfigPair>> getConfigValues) { | ||
| return Callbacks.grpcUnaryPromise(rpcCall).then(response -> { | ||
| String[][] result = new String[0][]; | ||
| getConfigValues.apply(response).forEach((item, index, array) -> { | ||
| result[index] = new String[] {item.getKey(), item.getStringValue()}; | ||
| return null; | ||
| }); | ||
| return Promise.resolve(result); | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public Promise<CoreClient> running() { | ||
| // This assumes that once the connection has been initialized and left a usable state, it cannot be used again | ||
|
|
@@ -42,7 +64,12 @@ public String getServerUrl() { | |
| } | ||
|
|
||
| public Promise<String[][]> getAuthConfigValues() { | ||
| return getConfigs( | ||
| c -> connection.get().configServiceClient().getAuthenticationConstants( | ||
| new AuthenticationConstantsRequest(), | ||
| connection.get().metadata(), | ||
| c::apply), | ||
| AuthenticationConstantsResponse::getConfigValuesList); | ||
| } | ||
|
|
||
| public Promise<Void> login(LoginCredentials credentials) { | ||
|
|
@@ -54,7 +81,12 @@ public Promise<Void> relogin(String token) { | |
| } | ||
|
|
||
| public Promise<String[][]> getServerConfigValues() { | ||
| return getConfigs( | ||
|
There was a problem hiding this comment. Just making sure - these are general server information, and we don't expect the user to be authenticated before retrieving them. There was a problem hiding this comment. Ah good catch - the server will have to auth this and fail the user in this case. i'll double check what dhe does here, but I think this is the same? There was a problem hiding this comment. Yes, on DHE it checks if you're authenticated for server config values: https://gitlab.deephaven.io/illumon/iris/-/blob/rc/silverheels/web/server-frontend/src/main/java/com/illumon/iris/web/server/WebApiServerImpl.java#L844 There was a problem hiding this comment. I'm leaving this as is, re-querying the server each time, to avoid the bug you describe. The tradeoff is that if the client asks for the config again, they will get an extra round trip, and in theory could get different values, going by the current implementation. That might be a feature too, letting the client poll for values. |
||
| c -> connection.get().configServiceClient().getConfigurationConstants( | ||
| new ConfigurationConstantsRequest(), | ||
| connection.get().metadata(), | ||
| c::apply), | ||
| ConfigurationConstantsResponse::getConfigValuesList); | ||
| } | ||
|
|
||
| public Promise<UserInfo> getUserInfo() { | ||
|
|
||
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.
Does Barrage provide
Implementation-Version? If not, should we add it there instead?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.
Ah, looks like it does, and this way is not used right now?
barrage=io.deephaven.barrage.flatbuf.BarrageMessageWrapper? I strongly lean towardsgetImplementationVersion().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.
Agreed, I just wanted more than one option to discuss, see description. I think this is the better option, it is slightly more generally adopted, but you still can't assume it is present.
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 wonder if there is a way we can have gradle output a "manifest" file of all the GAV coordinates it actually uses at runtime (in the context of jetty-server-app, etc).
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 discussed in slack)
Certainly possible, not even difficult - but it does but a burden on downstream consumers of the io.deephaven:server-jetty artifact to produce their own such artifact if they happen to manage any dependency differently that DHC itself specifies it. For example, if a bugfix release of barrage were to emerge that io.dh:server-jetty is compatible with, a project could specify that newer one in maven/ivy/sbt/gradle, but the version file wouldn't update unless we had scripts for their given build system.
In contrast, while the Package.getImplementationVersion() is not uniformly applied, it is part of Java itself, so a reasonable deployment should include it. In the event that a dependency doesn't include it but a project needs it, a "dummy" jar could be added to the project that exposes the dependency version, or set a regular configuration property and expose it through this service.