Bugzilla – Full Text Bug Listing
| Summary: | [engine] touchpoint prepare() called at the wrong time | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Jeff McAffer <jeffmcaffer> | ||||||
| Component: | p2 | Assignee: | Jeff McAffer <jeffmcaffer> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | pascal, simon_kaegi | ||||||
| Version: | 3.7 | ||||||||
| Target Milestone: | 3.7 RC1 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
Jeff any patch for this? Created attachment 195170 [details]
patch to reorder prepare call
Here is the straightforward fix. We move the prepare() call to before the phaseSet.perform() to conform to the API and merge the results etc.
HOWEVER, with the change there is a failure in the EclipseTouchpointTests.testInstallPartialIUValidationFailure. I don't really understand what is going on but it seems that some level of "validation" is failing (desirable in the test case) if the prepare is done after performing the phases but with the patch, the validation does not fail (bad in this testcase).
Pascal (or other) can you see what is going on here?
Simon, sorry to pull you back into this, but we would greatly appreciate if you could take a look into this. Thx. IIRC, the Engine follows the two phase commit model of DB (http://download.oracle.com/docs/cd/B28359_01/server.111/b28310/ds_txns003.htm). This means that the prepare happens after the work has actually been done, not before. In this case I think the javadoc for Touchpoint#prepare is incorrect and instead we need to introduce a new method that implements the behaviour described in Touchpoint#prepare. Yup. This would be fine with me as well. So an API clarification for 3.7 and potential new API for 3.8? I don't particularly think there is a need for force a new API in now. +1 for Doc change in 3.7 +1 for new API in 3.8 What Pascal said in comment 4 is right. The javadoc is not correct as "prepare" is a 2-phase-commit thing and is the touchpoint's last chance to validate the changes that have already happened in memory to the profile and potentially force a "rollback" before persisting them to disk on the "commit". With the current API there is no touchpoint "init" and instead what is done is any state is created lazily and then cleared on either commit or rollback. For example, see EclipseTouchpoint and what it does with the manipulators. Created attachment 195228 [details]
Proposed javadoc patch
The Javadoc update looks fine. It should be low risk as anyone actually using prepare() was seeing the newly described behaviour anyway. As for future direction, we could have an init() or some such, or, as Simon indicates, leave it to case-by-case lazy initialization. I don't mind that much either way though the lack of an init() is a little glaring give that there are so many other lifecycle hook points. Thx for the patch Simon! I've released it and opened bug #345348 to add an API in a future release. |
The JavaDoc for Touchpoint.prepare() says * This method is called at the beginning of an engine operation before any * phases have been executed. This is an opportunity to perform precondition * checks against the profile, or initialize any data structures common to an * entire execution of the engine. however the (condensed) code in Engine.perform() reads MultiStatus result = phaseSet.perform(session, operands, monitor); if (result.isOK() || result.matches(IStatus.INFO | IStatus.WARNING)) { result.merge(session.prepare(monitor)); Notice that phases are run (first line) and THEN prepare is (indirectly) called (last line). Its unclear what "prepare" means if it is run after all the work is done so it seems we should fix the code rather then change the Javadoc. Without the code fix there is no Touchpoint lifecycle that allows you to initialize the touchpoint itself. Not sure what the impact of changing the code would be. The EclipseTouchpoint has a prepare() method that does some validation stuff but its unclear what it really does. The Native touchpoint has the method but its just a super call. Of course, others in the community may have prepare()...