ADBA Early Thoughts/Notes

Tue Apr 17 19:02:11 UTC 2018

Chad,

Great feedback. Many thanks. Revisions will be in the next upload.

Replies in-line.

Douglas

> On Apr 17, 2018, at 9:59 AM, Chad Retz <chad.retz at gmail.com> wrote:
> 
> Hi. I wrote https://github.com/cretz/pgnio and am toying with
> implementing the latest ADBA interfaces and using the AoJ
> code as kind of a reference. Some of the early code can be seen in
> that repo. I haven't even gotten to a completed row
> operation yet, but I have had other unrelated items come up so I'm
> deprioritizing the work. But I jotted down some early
> notes and figured I'd share them. They are numbered for easy response,
> take 'em or leave 'em.
> 
> 1. What is DataSource.close expected to do?

It informs the DataSource that it will not be asked to create any more Connections. If the DataSource holds resources, it can/should release those resources. An example would be a DataSource that keeps a cache of Connections.

> 
> 2. Inconsistent parameter naming wrt DataSource.Builder.*Property and
> Connection.Builder.property

It seems redundant to include “connection” in the Connection.Builder method name. It is appropriate to use “connection” in the DataSource.Builder method names as a DataSource could have other kinds of properties, eg DataSource properties.

> 
> 3. Should the implementers ignore unknown connection properties? Do
> they have to still return them from
>   Connection.getProperties even if they have ignored them?

The philosophy is strict enforcement of limitations. So an implementation should reject  unknown or incompatible properties. That can either be when the property is first set or in the build method. JavaDoc fixed.

> 
> 4. Why is AdbaConnectionProperty.CONNECT_TIMEOUT a duration but
> AdbaConnectionProperty.NETWORK_TIMEOUT an int? What is
>   the unit of that int?

NETWORK_TIMEOUT should also be a Duration. Fixed.

> 
> 5. If connect timeout isn't present, is network timeout used or is
> there no connect timeout?

The NETWORK_TIMEOUT is used. The JavaDoc for all these needs a lot of work.

> 
> 6. Per-operation timeouts are probably not going to be easy to
> implement for most implementers. Should they be optional?
>   Otherwise, for things like Postgres, an entirely new connection has
> to be opened to "cancel" which would be
>   considered surprising behavior for the user. We can use the future
> timeout features in Java, but the connection is
>   still hung. Just curious what is expected of drivers here.

Every feature is optional. If it is impractical for an implementation to provide a specific feature, that implementation should not provide that feature. The goal is to provide an API for highly scalable, high throughput apps. Such apps do not want to do things that have excessive and unexpected impacts on performance. If an ADBA feature would require a lot of overhead then an implementation should not support it. It’s better that the app fail on the first test run as opposed to investing months in coding using a problematic feature only to discover in production that the performance is unacceptable.

> 
> 7. What if I call Operation.onError multiple times? Chained or
> replaced? AoJ doesn't allow even changing it which
>   seems limiting but fine if specified by the Javadoc.

Can only be called once. The JavaDoc has been fixed. Everything is write-once unless there is a specific reason it should be otherwise.

> 
> 8. Not clear which methods are thread safe. Assume none of them are
> unless otherwise mentioned like most docs? One might
>   expect Submission.cancel to be able to be called concurrently with
> other things on the connection.

This is definitely an open issue. My opinion is that most are not thread safe. The ones that are would be ones that are by design intended to be called simultaneously by multiple threads: DataSourceFactory.forName, DataSourceFactory.builder, DataSource.builder, Connection.abort, Submission.cancel. Possibly others but those are the ones I can think of right now.
> 
> 9. Why does SqlSkippedException need all of those values? Can it be
> specified in the docs what can be set to null and -1
>   and what not?

I have added a factory method that takes just a Throwable.
> 
> 10. Are there any complete impls of this spec? I think this should be
> a requirement before even submitting it for review
>    because so many things are found during impl phase. Maybe a
> closed-source Oracle one somewhere? Just trying to get
>    an idea of whether every method has been implemented somewhere
> even if not open source.

Not yet.
> 
> 11. How can a caller tell whether an operation group is parallel or
> not, dependent or not, has a condition, etc? Or is
>    it the caller's responsibility to know whether they have set those
> values previously?

The philosophy is that the app code should know what it is doing. There are very few methods that query things that the app should already know.

> 
> 12. For OperationGroup.independent, what does "f (sic) this {@link
> OperationGroup} blocks errors" means? (also, fix typo
>    there and typo on "iif" on the next method)

That’s a leftover from a different approach to error handling. Fixed/removed.

> 
> 13. Any chance on using a more public and friendly code review
> platform so I can just point out all the typos instead of
>    having to make lists of little things? There are so many little
> documentation inconsistencies. Not only typos.
>    Sometimes there isn't a space after punctuation. Sometimes javadoc
> refs aren't even to anything (e.g.
>    "{@link close()}" instead of "{@link #close()}"). Sometimes <p>
> used, sometimes not. Obviously none of them matter
>    really.

I have been told “no”.

> 
> 14. If OperationGroup.conditional is called multiple times, are the
> conditions chained or overridden? Assuming the
>    latter is the intent, the docs should say so.

throws IllegalStateException. JavaDoc has been fixed.  

The philosophy is to strictly enforce write-once. Any place there would be a question as to what it means to call some method twice the answer is almost certainly to throw IllegalStateException the second time. This should be specified in the JavaDoc. I’ll review to make sure it is.

> 
> 15. Class-level Javadoc for OperationGroup needs to explain what
> "held" means and its purpose. Wasn't immediately clear,
>    it just jumped into what to do when something is held.

That JavaDoc has been hard to write. I’ll keep working on it. I wish it wasn’t necessary but it is. 
> 
> 16. Need more docs on what happens, e.g., if
> OperationGroup.releaseProhibitingMoreMembers is called when not held.

throws IllegalStateException. Fixed the JavaDoc.
> 
> 17. What about protocols like Postgres that don't support multiplexing
> the socket for parallel operations? Parallel
>    essentially becomes a noop? Just looking for recommendations.

Not quite a noop. The implementation can reorder the Operations. If Operations have future parameters this can be beneficial.
> 
> 18. Why does the ArrayCountOperation interface re-define submit when
> there are no additional docs or covariant types?

copy paste error. Fixed/removed.
> 
> 19. In SqlException, why do the fields have default values and why not
> marked final?

Mistake. Fixed.
> 
> 20. Connection.connectOperation says it must be in NEW or NEW_INACTIVE
> when the op starts or it's a SqlException, but
>    then says it throws an IllegalStateException only if it isn't NEW.
> Which is it? Or is it both (check NEW on method
>    start and throw IllegalStateException, then NEW or NEW_INACTIVE on
> execution and throw SqlException)? What about
>    after the operation completes, should I check the lifecycle again
> before altering it?

The difference is between when the method is called and when it executes. It must be NEW when the method is called to create the connect Operation and either NEW or NEW_INACTIVE when the connect Operation executes. An inactive Connection continues to execute previously submitted work. Connect operations can only be executed on new (lower case) Connections. This JavaDoc is intended to capture those two requirements.
> 
> 21. Should an invalid connection on Connection.validationOperation
> throw a SqlException or an IllegalStateException or
>    is it up to me to throw any exception type I want even beyond those two?

If the Connection is not valid the Operation is completed exceptionally with an implementation specific Throwable. So far nothing is specified about what Throwables are used to complete failing Operations. The intent is that the implementation should use whatever best aids handling the error. We don’t want to require that implementations wrap an easily understood Throwable is some wrapper just to satisfy the spec. This doesn’t apply to SqlSkippedException as SqlSkippedException is very different from directly raising the cause of SqlSkippedException.

This needs to be documented, probably in the Operation class comment but maybe in submit and onError as well.
> 
> 22. Why does the connection have to be active to register a lifecycle listener?

Seemed like a good idea at the time. I could easily be convinced otherwise. 

The basic idea is that no one should be doing anything to an inactive Connection. If there are use cases for registering a listener on an inactive Connection there is no strong reason not to allow it. 
> 
> I would surely have a gazillion more notes as I actually got into
> implementing operations but I'll stop there (again,
> moving off the project a while). I will say overall the interfaces are
> more complex that I was expecting. I was
> expecting a more simple implementation around just async executing
> things and possibly returning responses instead of
> worrying about queuing, submission, etc. But either way, a target for
> async DB clients to target is very welcome.

I completely understand your comments about complexity. The API tries to minimize the complexity of the code that the app needs to actually get work done at the cost of more complexity in the API itself. An approach that would yield a simpler API would be to require the app to wire the Operations together with CompletionStages. It’s not that hard to do. We didn’t go down that path because the app ends up with a lot of boiler plate code that hurts readability and adds opportunities for bugs. Encapsulating that boiler plate code in the implementation simplifies the app code and makes it more reliable. Or at least that is the intent. The cost is a more complex API; the gain is an overall reduction in app complexity.

Again, many, many thanks for writing this up. I know how much time it took to go through both ADBA and AoJ and reach the level of understanding required to write these comments. Your effort is greatly appreciated.

Douglas
> 
> Chad