[crac] RFR: Fix ordering of invocation on Resources [v3]

[Anton Kozlov]

On Tue, 2 May 2023 10:41:36 GMT, Radim Vansa <duke at openjdk.org> wrote:

>> Just realized that this is required for PriorityContext implementation. But that is the implementation of that class, it's wrong ACI has to care about that.
>
> Yes, it's a bit of enforced flexibility of the base class (through allowing to override a method), though it doesn't need about the use case.
> 
> I could use just a list rather than sub-contexts but that would require duplicated code.

That would be a cleaner approach compared to interdependencies of ACI-PC (details of PC leaks to ACI)

>>> The contexts are supposed to run all resources regardless of whether any failed, therefore there's no point in propagating and re-wrapping the exceptions. ...  I don't see why would the parent context decide on the type - because it would get just CheckpointException with a list of other failures from components it does not understand.
>> 
>> The parent Context may implement an artbitrary handling (for example, unloading a component compleltely if that is throwing an exception). So throwing an exception is something useful.
>> 
>> With that, the new Core.recordException is completely new exception flow, that just opimizes somthing the generic throw scheme. With that, the generic schemes should be something good enough already, we don't need to complicate the interface, the code,.. CheckpointExcepotions are still exceptions, that is, we don't expect them often, there is no need to optimize them.
>
> It's not about optimization (in the sense of performance) but about removing code bloat, and the need for parent context to tediously copy failures (deciding whether something was a wrapper exception or the actual failure), when all you need to do in the end is to report them in bulk. It's not a new exception flow, it's removing the flow as there is no exception flow needed.
> 
> Your example is invalid: If a throwing resource is to be removed, it's the task of the parent context - and that one will see the exception. The parent context should not remove its child context since one of the N resources in the child context is failing.

You're trading additional code for a complicated interface. What are the directions what to use: Core.recordException() vs throw new Exception() ?  This two very similar interfaces are the sign we are trying to do something strange.

> Your example is invalid: If a throwing resource is to be removed, it's the task of the parent context - and that one will see the exception. The parent context should not remove its child context since one of the N resources in the child context is failing.

Could you elaborate?.. I'm lost what "remove child context" means. I was reffering to the parent context being able to handle a CheckpointException from the child context.

>> I'm trying to describe class hierachy and failing. The patch tries to reverse ACI-PC relation. ACI was for partially-ordered Resources (defined by a Comparator), and now it's for totally-ordered Resources (ordered by long). Trying to fit the partially-ordering PC as a subclass of the totally-ordering ACI feels unnatural. Can we have a cleaner hierachy of the classes?
>
> No; ACI was totally-ordered in the previous version of the PR, but now it doesn't care about ordering at all; it's the abstract `runBeforeCheckpoint` that decides on the order.

I see, thanks. So ACI task is to track beforeCheckpoint order and provide afterRestore in the opposite order, this seems the most useful part of it. But its interface does not seem to be very consistent.

 Does it really needs an abstract base class that also maintains calling, logging and exception propagation? It will be nice to separate all this concerns for greater flexibility e.g. with composition of some classes in a Context implementation, rather than that to extend the base class with all of them combined.

-------------

PR Review Comment: https://git.openjdk.org/crac/pull/60#discussion_r1185090313
PR Review Comment: https://git.openjdk.org/crac/pull/60#discussion_r1185113136
PR Review Comment: https://git.openjdk.org/crac/pull/60#discussion_r1185223155