First impression after reviewing the current API

[Jens Schauder]

Hi Douglas,

regarding reactive Flow integration:

I think what I would like to see is methods taking a SQL statement and a
Publisher of parameter bindings and returning a Publisher of Results or a
Publisher of Publisher of Results (for multiple SQL executions where each
produces many Rows). That is just a first idea and I guess the underlying
question for me is: Are you aiming "just" for an async API? Or do you want
a "reactive" API or both?


Regarding
> No member Operation is submitted until after the OperationGroup is
submitted. Calling any initialization method such as memberAggregator after
the OperationGroup is submitted throws IllegalStateException.
Ok. I thought it was intended the other way round due to this JavaDoc
comment on OperationGroup:

* If an {@link OperationGroup} is held additional member {@link Operation}s
may

* be submitted after the {@link OperationGroup} is submitted.

If you can submit element operations only after submitting the group
non-held groups seem to be useless.

If there is such a strict separation between what is possible before and
after submitting an Operation, I'd like to see separate interfaces for the
two states.


Jens


On Wed, Oct 4, 2017 at 7:14 PM, Douglas Surber <douglas.surber at oracle.com>
wrote:

> Jens,
>
> Thanks for your comments.
>
> PublisherOperation is a leftover and should not be there. It was part of a
> failed attempt to integrate reactive stream handling for Rows. I guess I
> created that interface before I branched.
>
> Making Row extend Flow.Subscription is indeed a bit odd. The problem is
> that I can’t identify any entities that correspond to Publisher and
> Subscriber. Something needs to expose the cancel and request methods.
> Ignoring the existence of Flow, there doesn’t seem to be any problem with
> putting them in Row. So it works to put them on Row and make Row extend
> Subscription. Again, this is a work in progress so feel free to suggest an
> alternative.
>
>
> Other replies inline below.
>
> Douglas
>
> > On Oct 4, 2017, at 8:41 AM, Jens Schauder <jschauder at pivotal.io> wrote:
> >
> > Hi,
> > great to see that this API moves forward and gets developed in the open.
> >
> > I went through the Async JDBC API and tried to understand how it might
> get
> > used. Especially with regards to the Flow integration.
> > I have a hard time understanding how that integration is supposed to
> work.
> > Let's start with some details:
> >
> > - `OperationGroup` has various methods to create `Operation`s that each
> > takes a SQL String as an argument. There is `rowOperation` which returns
> a
> > `ParameterizedRowOperation`, which allows to set bind parameters and is
> > intended to produce `Row`s.
> > So I guess this would be the one to use for everyday SELECT statements.
> > But if I want to use an Operation as a `Publisher` I would need to call
> > `publisherOperation` but then I can't provide parameters.
> > Let alone parameters provided by one or more `Publisher`s
> >
> > - `Result.Row` extends `Flow.Subscription` but a `Subscription` should be
> > the result of a `Subscriber` subscribing to a `Provider`.
> > Having many (one for each row) `Subscription`s that nobody subscribed for
> > seems strange and confusing.
> > It also seems to redefine the specification of `Subscription` by allowing
> > implementations to ignore calls to `request`.
> >
> >
> > Overall I have the impression that multiple separate things are getting
> > crammed into the `Operation` inheritance hierarchy:
> > - How to specify the SQL statement to be executed (via
> > `ParameterizedRowOperation`)
> > - How to consume the results (via `PublisherOperation`)
> > - How to aggregate results (via `RowOperation.rowAggregator`)
> >
> > Part of the reason for this might be that the API is partially based on
> > `Future`s and partially on `Flow`s.
> > I think one might arrive at a cleaner API if one goes all out for one of
> > the approaches, and leaves the conversion between the worlds to other
> APIs.
>
> >
> >
> > Some other minor things I noted/do not understand:
> >
> > - Operation.timeout throws an exception when called more than once. Why
> not
> > just use the last value? This behavior makes cascading configuration
> > unnecessarily difficult, where one part of the code receives an
> > `Operation`, without being informed about if there was already a timeout
> > set.
>
> One of the design guidelines for the entire API is objects are immutable
> after initialization. Allowing objects to be reinitialized complicates both
> the spec and the implementation. Perhaps allowing reinitialization of the
> timeout would be easy to specify and implement but then timeout is an
> exception to the rule and exceptions are not good. So we stick to the basic
> principle that values are set once and attempting to reset them is an error.
>
> > - what is a SQL format mentioned in `DataSource.translateSql` and
> > `supportedTranslateSqlFormats`?
>
> The API is a work in progress. This is just an idea to address the
> limitation of supporting only vendor specific SQL. The notion is that some
> vendors may want to support other kinds of SQL, ODBC for example. This
> would provide a standard hook for those vendors to support other kinds of
> SQL without cluttering the rest of the API or implementation.
>
> > - What does public <T> T ResultMap.get(String id, Class<T> type); return
> if
> > the value identified by id cannot be converted to the type indicated by
> > type?
>
> How about it throws ClassCastException?
>
> > - The complete lack of metadata seems to be problematic. How does one
> > obtain data in a usable generic way if nothing of it's type can be known?
> > One might pass in Object.class but the return type is then completely
> > becomes vendor specific, and might be some internal representation. I
> think
> > some minimal metadata like the preferred/the available types either as
> Java
> > types or as SqlTypes should be available.
>
> As with everything else this is open for discussion. If you can provide a
> compelling use case for a significant fraction of high throughput apps
> needing some metadata it would be considered. But this is a high bar.
>
> > - public OperationGroup<S, T> memberAggregator(BiFunction<T, S, T>
> > aggregator); Forces an `OperationGroup` to keep all results of all
> > contained `Operation`s until this method is called or the the group is
> > completed.
> > Maybe it should throw an exception if it gets called after the first
> > Operation is submitted?
>
> No member Operation is submitted until after the OperationGroup is
> submitted. Calling any initialization method such as memberAggregator after
> the OperationGroup is submitted throws IllegalStateException.
>
> >
> > Kind regards
> > Jens Schauder
>
>