First impression after reviewing the current API

[Douglas Surber]

The intent is an asynchronous API. There is a need for back pressure in two places, submitting Operations and fetching rows. Aside from those to special cases reactivity is not a goal.

Submitting additional Operations to an OperationGroup does not require storing results for later processing by a ResultAggregator. Submitting additional Operations after the OperationGroup is submitted is a special case and requires special handling, i.e. holdForMoreMembers and releaseProhibitingMoreMembers. In the typical case submitting additional members after submitting the group is an error.

> On Oct 6, 2017, at 1:13 AM, Jens Schauder <jschauder at pivotal.io> wrote:
> 
> 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
>> 
>>