Skip to content
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

Add resolve URI interface for local tables, remote barrage subscripti… #1472

Merged
merged 16 commits into from
Oct 28, 2021

Conversation

devinrsmith
Copy link
Member

…ons, read csv, and parquet tables

@devinrsmith devinrsmith marked this pull request as ready for review October 21, 2021 23:47
@devinrsmith
Copy link
Member Author

import static io.deephaven.db.tables.utils.ResolveTools.resolve

trades = resolve('dh+plain://104.198.75.160/scope/trades_aggregation')
quotes = resolve('dh+plain://34.69.133.75/scope/quotes')

quotes_and_trades = merge(
        quotes.view("Exchange", "Instrument"),
        trades.view("Exchange", "Instrument"))
    .selectDistinct()
    .naturalJoin(quotes, "Exchange,Instrument", "QuoteTimestamp=Timestamp,Bid,Ask")
    .naturalJoin(trades, "Exchange,Instrument")
    .sort("Instrument", "Exchange")

Test script. Will keep the servers up until this merges in.

@devinrsmith
Copy link
Member Author

Follow up issues, #1482, #1483

nbauernfeind
nbauernfeind previously approved these changes Oct 26, 2021

public interface Builder {

Builder host(String host);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not quite following this builder wiring and where this belongs, but host must be validated to never have : or / in it (or ?)

i think this is true for a few others too (port is safe since it is an int, etc)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - the validation actually happens in the check by constructing the actual URI and then comparing it to the input:

    @Check
    final void checkHostPort() {
        // Will cause URI exception if port is invalid too
        if (!host().equals(toUri().getHost())) {
            throw new IllegalArgumentException(String.format("Invalid host '%s'", host()));
        }
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add more tests to verify.

*
* @return if TLS is enabled
*/
public abstract boolean isTLS();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i understand the relationship to the strings above, but what does this actually mean? there doesn't seem to be any actual transport in this PR that does work (or not) on that flag?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: if a "remote uri" has type name "RemoteUri", this should probably be "isTls"? or rename remote uri to RemoteURI and toUri to toURI?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io.deephaven.client.impl.ChannelHelper#channelBuilder(DeephavenTarget) does the work


@Check
final void checkApplicationId() {
if (!UriHelper.isUriSafe(applicationId())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to assert that applicationId cannot end with /field, etc, right? possibly never allow slashes in any of these properties

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isUriSafe checks that. I'll add a suite of UriHelper tests.

*
* @return the scheme
*/
String scheme();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to expose other URI notions like authority and path here as well, and handle the tostring aspects at this level, so as avoid repeating %s://%s/%s at each level and only offer the "how do i make a path" based on the specific context?

it does seem weird to have in some implementations

  • a method that returns the constant MY_AUTHORITY
  • a String.format(... call with %s for the authority, as a placeholder, and
  • as a parameter to that format() call, actually pass the constant MY_AUTHORITY again, rather than call authority() (or, just inline it in the format string)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can pull out some common code to DeephavenUriBase for toURI IMO, but the Deephaven URIs themself prefer to present themselves in their more strongly typed form; and there isn't a need to dip down into the actual URI components atm. And actually looking, I'm not even using scheme() so I can get rid of that.

Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tentative approval, a few things I'd ask for in descending order of how much I want them:

  • Make naming acronyms consistent: TLS vs Id, Uri.
  • Documentation so that others don't have to figure out the "why" of this. It doesn't have to be official docs, consider a readme.md in java-client/uri, or a wiki page, or a new discussion started so that this can evolve more easily as use cases are presented.
  • Break it out as an extension to be enabled, so as to not depend on it for every setup, avoid making it integral at this point (as a better/simpler/clearer window into things that could be achieved in other ways)
  • Super mildest of all, split out the not-URI/Uri stuff like FlightSession.schema, SchemaHelper, session token refresh, table handle/ticket cleanup into its own PR so it is its own commit

@devinrsmith
Copy link
Member Author

I've tried to be a bit more consistent with naming. Added some documentation about URI and server side of things. Hoping to punt on the enabling / configuration of URI resolution until it's exposed to gRPC. Will add info to #1483

@devinrsmith devinrsmith merged commit d78f497 into deephaven:main Oct 28, 2021
@devinrsmith devinrsmith deleted the resolvable-uris branch October 28, 2021 01:38
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants