OperationGroup extends AutoCloseable

[Mark Rotteveel]

I have to say I find these parts of ADBA a bit confusing, and I wonder 
if these kind of rules don't make it too complex and prone to incorrect 
use. Unfortunately, I still haven't had the time or energy to do a real 
deep dive into all parts of ADBA, so I can't really offer an insightful 
alternative yet.

For one, I don't really understand what an `OperationGroup` is supposed 
to represent, and how it is different from just a `Connection` (ie: why 
is it modeled separately), and why it should be 'submittable' at all. I 
also don't understand why you can do more things to it after a 
`submit()`, as submit sounds like a final operation to me. On the other 
hand, as the code below demonstrates, not accepting new operations would 
lead to usage errors. It sounds a bit like a transaction, but then 
`Transaction` is modeled separately.

Unfortunately, the javadoc on `OperationGroup` doesn't help much in 
understanding it.

Mark

On 2018-07-19 21:01, Douglas Surber wrote:
> Yes, all the calls are non-blocking. The close doesn’t “terminate the
> group”. It notifies the implementation that no more member Operations
> will be added. It doesn’t stop anything from executing.
> 
> What I didn’t discuss but should have is the case where members are
> added as a result of the execution of other Operations.
> 
>   OperationGroup grp = session.operationGroup();
>   grp.submit();
>   grp.rowOperation(selectSql)
>     .collect(Collector.of( () -> null,
>                                     (n, c) -> 
> grp.rowCountOperation(insertSql)
>                                                      .set(“param”,
> c.at <http://c.at/>(“index”).get(Integer.class), AdbaType.INTEGER)
>                                                      .submit(),
>                                    (l, r) -> l,
>                                    n -> { grp.close(); return n;}))
>      .submit();
> 
> The above does an insert for each row selected. The OperationGroup
> cannot be closed by try with resources because it would be closed
> before all (any) of the inserts are added. The problem with this
> pattern is failing to call grp.close() in the finalize method. If that
> happens the OperationGroup will never be completed and execution will
> hang.
> 
> This was the code pattern that motivated the ability to add members
> after submit, but I don’t believe it will be the most common use case.
> It’s unfortunate that it doesn’t fail overtly rather than hanging, but
> the current design is no better in this regard. It just has more
> explicit method names. I don’t think those method names carry their
> weight compared to using try with resources.
> 
> You can write exactly the code above with a RowPublisherOperation. It
> takes a few more lines of code to define the Subscriber, but it’s not
> hard.
> 
> Douglas
> 
>> On Jul 19, 2018, at 11:11 AM, Dávid Karnok <akarnokd at gmail.com> wrote:
>> 
>> Aren't those submit calls (supposed) asynchronous? In that case, won't 
>> the try-with-resources terminate the group before, most likely, 
>> anything could even happen?
>> 
>> The scenario sounds like typical multi-valued asynchronous flow, for 
>> which a Flow.Publisher<Submission> source would be able to generate 
>> more submissions as well as indicate there won't be any further 
>> submissions.
>> 
>> Douglas Surber <douglas.surber at oracle.com 
>> <mailto:douglas.surber at oracle.com>> ezt írta (időpont: 2018. júl. 19., 
>> Cs, 19:46):
>> OperationGroup has a problem. It needs to be possible to add more 
>> member Operations to an OperationGroup after it is submitted. It is 
>> also necessary to know when all member Operations have been added as 
>> the OperationGroup cannot be completed until it knows no more member 
>> Operations will be added. Finally we want to minimize the likelyhood 
>> of Session execution hanging because an OperationGroup is waiting for 
>> more members when there are none.
>> 
>> I approached this problem by specifying that by default no more member 
>> Operations can be added after the OperationGroup is submitted. For the 
>> case where members need to be added after submission there are two 
>> special methods.
>> 
>> public Submission<T> submitHoldingForMoreMembers();
>> public Submission<T> releaseProhibitingMoreMembers();
>> 
>> The long, descriptive names are intentional. The goal is to clearly 
>> remind users that they must release the OperationGroup if they hold 
>> it. I’ve never liked this approach.
>> 
>> I’d like to offer an alternative.
>> 
>> - Remove submitHoldingForMoreMembers and 
>> releaseProhibitingMoreMembers.
>> - Define OperationGroup to implement AutoCloseable.
>> - Member Operations can be added to an open OperationGroup.
>> - An OperationGroup will not be completed until after it is closed.
>> - An OperationGroup must be submitted before it is closed.
>> 
>> try (OperationGroup grp = session.operationGroup()) {
>>   grp.rowCountOperation(insertSql)
>>      .submit();
>>   CompletionStage result = grp.submit().getCompletionStage();
>>   grp.rowCountOperation(updateSql)
>>      .submit();
>> }
>> 
>> A single usage pattern using try with resources seems much more 
>> reliable that the multiple usage patters with unfamiliar methods. Yes, 
>> the default case
>> 
>>   OperationGroup grp = session.operationGroup();
>>   grp.rowCountOperation(insertSql)
>>      .submit();
>>   grp.rowCountOperation(updateSql)
>>      .submit();
>>   return grp.submit().getCompletionStage();
>> 
>> is perhaps simpler, but not by much.
>> 
>> try (OperationGroup grp = session.operationGroup()) {
>>   grp.rowCountOperation(insertSql)
>>      .submit();
>>   grp.rowCountOperation(updateSql)
>>      .submit();
>>   return grp.submit().getCompletionStage();
>> }
>> 
>> I’m starting to believe that the default case is not going to be the 
>> most common case. I think held OperationGroup are going to be much 
>> more common than I initially believed. A “held” group would look very 
>> similar to the “default” case.
>> 
>> try (OperationGroup grp = session.operationGroup()) {
>>   CompletionStage result = grp.submit().getCompletionStage();
>>   grp.rowCountOperation(insertSql)
>>      .submit();
>>   grp.rowCountOperation(updateSql)
>>      .submit();
>>   return result;
>> }
>> 
>> The only difference is when the OperationGroup is submitted. If 
>> OperationGroups are always written using try with resources the 
>> forgetting to close one will be rare and easily noticed in code 
>> reviews. The tiny advantage of the default case, not having to call 
>> close, doesn’t seem to buy much compared to the complexity the current 
>> model adds to the held case. Uniform use of try with resources seems a 
>> much better approach.
>> 
>> Thoughts?
>> 
>> Douglas
>> 
>> 
>> 
>> 
>> --
>> Best regards,
>> David Karnok