Bugzilla – Full Text Bug Listing
| Summary: | Revert objects to CLEAN if new state matches clean state | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Caspar D. <caspar_d> | ||||||
| Component: | cdo.core | Assignee: | Project Inbox <emf.cdo-inbox> | ||||||
| Status: | CLOSED DUPLICATE | QA Contact: | Eike Stepper <stepper> | ||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | alex.lagarde, cyril.jaquier, esteban.dugueperoux, saulius.tvarijonas, stephane.fournier, steve.monnier, vroldanbet | ||||||
| Version: | 4.3 | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 352977, 359064 | ||||||||
| Attachments: |
|
||||||||
|
Description
Caspar D.
I agree. Do you want to work on it? Yep. Assigning to myself. Note: I realized that the failure of CDO to identify the object's dirty state as matching its original clean state, does not only inconvenience client-side tools such as EMF commands, but also has the undesirable consequence that new but identical revisions are created on the server side when such objects are committed. We have no pre-commit check in place that checks whether the net effect of a set of feature deltas for an object, is perhaps nil... Will provide testcase shortly demonstrating both aspects of the problem. Created attachment 171621 [details]
Testcase (as a patch)
I've been thinking about this, and it doesn't seem as straightforward to address as I initially thought. The savepoints are a serious headache. If A.x == 1 initially (i.e. in its clean state), and then we set A.x=2, then set a savepoint S, then set A.x=1 again, we probably want it to appear as clean. But what about the deltas (there would be 2 in this example obviously)? We can't clear them because the user app might still call rollback(S), which needs to restore the intermediate dirty state. This can only be done using the deltas. This suggests that the deltas as used by savepoints should probably be viewed as different from deltas that actually get submitted to the server in a commit. The latter don't need to (and should not) transmit a sequence of deltas that replays irrelevant intermediate dirty states (but currently they do). Recomputing the deltas during precommit to a "lean" set is doable, although it would require the TX to hold on to the clean revisions that were loaded originally, for every object that becomes dirty. (They form the baseline against which the lean deltas can be computed.) I'm wondering if this will significantly affect performance for big commits. A second problem is that the semantics of rollback will become a bit weird. Using the same example, say that object A transitions to CLEAN again when we set A.x=1. Rolling the tx back to S must necessarily put it in the DIRTY state again. So, rolling back a clean object to a savepoint, might make it dirty... Rebasing all outstanding enhancements requests to version 4.0 (In reply to comment #5) > [...] > > This suggests that the deltas as used by savepoints should > probably be viewed as different from deltas that actually > get submitted to the server in a commit. I think CDOTransaction.getRevisionDeltas() should reflect what gets committed to the server. That's equivalent to CDOSavepoint.getAllRevisionDeltas() but may be different from CDOSavepoint.getRevisionDeltas(). (In reply to comment #7) I don't see how your comment addresses either of the aspects of the problem, which I'll restate: (1) deltas can't be netted out of the savePoints' collections (because that would make rollback to the state designated by the savePoint impossible), but at the same time we do want an object to revert locally to CLEAN when the net effect of its deltas is nil; and (2) while deltas cannot be netted out from the savePoints, they *should* be netted out from the changeSet that gets sent to the server. Actually (2) can be fixed with something like getAllRevisionDeltas(boolean net), a call to which could perform the netting during preCommit. But (1) is more problematic, because it implies that an object can be clean even though there are deltas present for it in the union of savePoints' delta collections (and moreover, as stated before, that rollbacks can actually make an object go from CLEAN to DIRTY). And it also again raises the question: what is CLEAN, exactly, i.e. what is the basis for comparison when trying to determine whether a set of deltas have zero net effect? If "clean" means "latest known server state", that is at odds with the idea that a savePoint references a local state (which obviously it's meant to). If "clean" means "whatever the client loaded initially", that's at odds with passiveUpdates. (In reply to comment #8) > I don't see how your comment addresses either of the aspects of the > problem, which I'll restate: > > (1) deltas can't be netted out of the savePoints' collections (because > that would make rollback to the state designated by the savePoint > impossible), but at the same time we do want an object to revert locally > to CLEAN when the net effect of its deltas is nil; > > and > > (2) while deltas cannot be netted out from the savePoints, they *should* > be netted out from the changeSet that gets sent to the server. That's what I meant to say. CDOSavepoint.getAllRevisionDeltas (==CDOTransaction.getRevisionDeltas ==commit changeset == CDOObject state) may be different (i.e. netted out) from CDOSavepoint.getRevisionDeltas (i.e. not netted out to support rollback). I must admit that I can't remember what exactly CDOSavepoint.getRevisionDeltas is supposed to contain, the changes that happened after setting this save point or an aggregation of the changes of all save points? > Actually (2) can be fixed with something like > getAllRevisionDeltas(boolean net), a call to which could perform the > netting during preCommit. Why not always netting out in this list (see above)? > But (1) is more problematic, because it implies that an object can be > clean even though there are deltas present for it in the union of > savePoints' delta collections ... That sounds problematic. >(and moreover, as stated before, that > rollbacks can actually make an object go from CLEAN to DIRTY). Conceptually that sounds right. > And it also again raises the question: what is CLEAN, exactly, i.e. what > is the basis for comparison when trying to determine whether a set of > deltas have zero net effect? With this bugzilla fixed, an object is CLEAN exactly if its current values are equal to the values that it had after the last load or commit operation. > If "clean" means "latest known server > state", that is at odds with the idea that a savePoint references a > local state (which obviously it's meant to). If "clean" means "whatever > the client loaded initially", that's at odds with passiveUpdates. I see. Am I right that it's the rollback() method that is conceptually problematic with passiveUpdates? Either way we define the semantics/effect of a rollback we seem to have an issue. Would a parameter on the rollback method be a good solution? so that the user can decide whether he wants to rollback to the latest server state or the latest local state before the transaction became dirty. What about rollbacks to intermediary save points? (In reply to comment #9) > That's what I meant to say. CDOSavepoint.getAllRevisionDeltas > (==CDOTransaction.getRevisionDeltas ==commit changeset == CDOObject state) may > be different (i.e. netted out) from CDOSavepoint.getRevisionDeltas (i.e. not > netted out to support rollback). > > I must admit that I can't remember what exactly CDOSavepoint.getRevisionDeltas > is supposed to contain, the changes that happened after setting this save point > or an aggregation of the changes of all save points? The former. > > Actually (2) can be fixed with something like > > getAllRevisionDeltas(boolean net), a call to which could perform the > > netting during preCommit. > > Why not always netting out in this list (see above)? I'm not sure if all current usage of getAllRevisionDeltas would continue to function correctly. But intuitively it seems that it should... > With this bugzilla fixed, an object is CLEAN exactly if its current values are > equal to the values that it had after the last load or commit operation. OK. > > If "clean" means "latest known server state", that is at odds with > > the idea that a savePoint references a local state (which obviously > > it's meant to). If "clean" means "whatever the client loaded > > initially", that's at odds with passiveUpdates. > > I see. Am I right that it's the rollback() method that is conceptually > problematic with passiveUpdates? Yes. > Either way we define the semantics/effect of a rollback we seem to > have an issue. Exactly. > Would a parameter on the rollback method be a good solution? so that > the user can decide whether he wants to rollback to the latest server > state or the latest local state before the transaction became dirty. Yep, I think that's a possible solution. > What about rollbacks to intermediary save points? That's the real can of worms. What to do if a passiveUpdate was received at time T, and the user wants to roll back to an intermediate savePoint set at a time S prior to T? Restoring the state of the objects that were affected by the passiveUpdate, to their state at S, means we undo the passiveUpdate. And the state state at S might still be DIRTY, and therefore committable -- so effectively such a local rollback can potentially undo parts of a commit performed by another user. And worst: there is no way of detecting this situation. (Even if an app contains a hand-coded mechanism that tracks which objects were touched by passiveUpdates, I don't think we have a hook into rollbacks...) There is in fact a simple solution: we disallow rollbacks to savePoints set prior to the processing of a passiveUpdate. Crude and effective, but users might not like it, because they would lose the guarantee that a savePoint, once set, remains available as a rollback target until the next commit. A slightly more sophisticated solution would actually check if the rollback and the passiveUpdate conflict in any way. But that would require passiveUpdate data to be kept in the session after it's been processed, and besides, still suffers from the same drawback as the cruder option above, except that it would occur less often. A third solution would be to re-apply passiveUpdates after the rollback. This too requires the passiveUpdate data to be kept, which could be a big amount of data when there are many users. Besides, it's complicated, as it would require re-invocation of the merger, which may fail where it succeeded the first time it processed the update, leaving conflicts after the re-application of old updates. Your head spinning yet? Mine is :-) *** Bug 329753 has been marked as a duplicate of this bug. *** I'm facing the same problem. Let's say an EMF command modifies a clean object. The change is only a dummy attribute new value. I assume no other changes happen. I only want to undo my modification. How to implement the undo to revert the cdo object to its clean state rather than having the cdo object in a dirty state ? This bugzilla seems to address the same issue. Do you plan a fix a workaround ? Stephane (In reply to comment #12) > This bugzilla seems to address the same issue. Yes. > Do you plan a fix a workaround ? If you == Caspar and plan == "have an idea of when", then the answer is no, so I'll unassign myself from this for now. However, I do still consider this an important issue. I expect it'll be quite a bit of work to address it. As I've pointed out, serious complications arise from the local savePoints, and from passiveUpdates/refreshes, see earlier comments. (In reply to comment #13) > (In reply to comment #12) > > This bugzilla seems to address the same issue. > > Yes. > > > Do you plan a fix a workaround ? > > If you == Caspar and plan == "have an idea of when", then the answer is no, > so I'll unassign myself from this for now. > > However, I do still consider this an important issue. I expect it'll be quite > a bit of work to address it. As I've pointed out, serious complications arise > from the local savePoints, and from passiveUpdates/refreshes, see earlier > comments. Is there a way for me to be able to revert to clean state with current CDO API from dirty state ? I understand complications arise from local savePoints.... But I'm trying to find out a solution for my use case first. (In reply to comment #14) I think at the moment the cleanest (but not the lightest) solution is to set a CDOSavepoint after each modification that you want to rollback() to. I'd also prefer a more transparent and less heavy method but that would definitely a major engineering task ;-( Moving all open enhancement requests to 4.1 Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master. On revisiting this issue it strikes me that we could eventually remove ALL DELTAS from the savepoints in favour of storing revision snapshots. The main reason we store actual deltas was that we originally considered this faster than comparing snapshots at commit time to compute deltas on demand. Consequently we didn't have revision comparisons at that time (it came in later for other requirements). I believe that our current revision comparison method is fast for (almost?) all cases. The impact of this change to the code base is probably huge but it would certainly make it much simpler. Any thoughts? Created attachment 225103 [details]
Another test case
Moving all outstanding enhancements to 4.3 *** This bug has been marked as a duplicate of bug 400311 *** |