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,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) {} | ||
| } | ||
|
|
||
| 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. |
||
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)