Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 312534 - Revert objects to CLEAN if new state matches clean state
Summary: Revert objects to CLEAN if new state matches clean state
Status: CLOSED DUPLICATE of bug 400311
Alias: None
Product: EMF
Classification: Modeling
Component: cdo.core (show other bugs)
Version: 4.3   Edit
Hardware: All All
: P3 enhancement with 2 votes (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact: Eike Stepper CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 352977 359064
  Show dependency tree
 
Reported: 2010-05-12 03:02 EDT by Caspar D. CLA
Modified: 2014-01-09 13:57 EST (History)
7 users (show)

See Also:


Attachments
Testcase (as a patch) (5.97 KB, patch)
2010-06-10 05:47 EDT, Caspar D. CLA
no flags Details | Diff
Another test case (2.50 KB, patch)
2012-12-31 04:44 EST, Eike Stepper CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caspar D. CLA 2010-05-12 03:02:35 EDT
Say one loads an object A with a value of 1 for attribute x.
A will be CLEAN. Say one then assigns A.x=2; this causes A
to transition to DIRTY. Then, say one assigns A.x=1. A will
still be DIRTY even though its state is now identical again
to its persisted "current" revision.

It would be useful to have a way of detecting that A's state
is now identical to its persisted state again, and of having
it therefore revert to state CLEAN (and to using the
persisted revision instead of the transactional revision).

This is useful, for example, when using EMF Commands, the
purpose of which is (among other things) to support
undo/redo of operations on EObjects. Currently, if one
undoes an EMF command that operated on a CDOObject, the
CDOObject will remain DIRTY even if the undo-op restores
the object's state to what it was when it was cleanly
loaded. From the perspective of an EMF Command user, that
will seem strange.
Comment 1 Eike Stepper CLA 2010-05-18 06:38:05 EDT
I agree. Do you want to work on it?
Comment 2 Caspar D. CLA 2010-05-19 23:28:19 EDT
Yep. Assigning to myself.
Comment 3 Caspar D. CLA 2010-06-10 05:42:53 EDT
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.
Comment 4 Caspar D. CLA 2010-06-10 05:47:53 EDT
Created attachment 171621 [details]
Testcase (as a patch)
Comment 5 Caspar D. CLA 2010-06-16 06:52:43 EDT
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...
Comment 6 Eike Stepper CLA 2010-06-29 04:49:47 EDT
Rebasing all outstanding enhancements requests to version 4.0
Comment 7 Eike Stepper CLA 2010-07-26 03:49:45 EDT
(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().
Comment 8 Caspar D. CLA 2010-07-27 03:03:48 EDT
(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.
Comment 9 Eike Stepper CLA 2010-07-27 03:34:41 EDT
(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?
Comment 10 Caspar D. CLA 2010-07-27 06:51:05 EDT
(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 :-)
Comment 11 Eike Stepper CLA 2010-11-27 01:55:27 EST
*** Bug 329753 has been marked as a duplicate of this bug. ***
Comment 12 Stephane fournier CLA 2011-06-09 04:39:16 EDT
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
Comment 13 Caspar D. CLA 2011-06-09 05:28:34 EDT
(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.
Comment 14 Stephane fournier CLA 2011-06-09 07:57:07 EDT
(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.
Comment 15 Eike Stepper CLA 2011-06-09 14:24:18 EDT
(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 ;-(
Comment 16 Eike Stepper CLA 2011-06-23 03:56:07 EDT
Moving all open enhancement requests to 4.1
Comment 17 Eike Stepper CLA 2012-08-14 22:49:45 EDT
Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master.
Comment 18 Eike Stepper CLA 2012-12-31 04:42:10 EST
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?
Comment 19 Eike Stepper CLA 2012-12-31 04:44:50 EST
Created attachment 225103 [details]
Another test case
Comment 20 Eike Stepper CLA 2013-06-27 04:04:56 EDT
Moving all outstanding enhancements to 4.3
Comment 21 Eike Stepper CLA 2014-01-09 13:57:25 EST

*** This bug has been marked as a duplicate of bug 400311 ***