First impression after reviewing the current API

[Douglas Surber]

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