As a performance optimization, we don't want to version
live items in CMS. The current system doesn't version
live items. All interested parties (i.e. Dan and David)
agree that this behavior should be preserved in the new
system, i.e. live items should not be versioned.
The way this is currently achieved on the tip is, we call
Versions.suspendVersioning() on entry into the publish()
and unpublish() methods of ContentItem. This is based
on the assumption that nothing that happens inside
the publish() and unpublish() methods should be versioned.
This assumption strikes some as a bit too risky,
an assessment with which I basically agree.
The proposed compromise is to not suspend versioning
in the publish method. This adds a trivial overhead
on the part of the versioning service, because publishing
mostly involves creating new objects. For newly created data
objects, the versioning service does not record anything
other than the fact that the object was created. So, the
overhead imposed by versioning is basically one extra
insert statements per each data object (plus some constant
overhead of less than 10 inserts per txn). This is negligible
in the grand scheme of publishing.
Unpublishing, on the other hand, involves deletion of content
items. This is expensive from the versioning point of view,
because we must record enough information to undelete all
the deleted content items. After hashing this out with
Dan and David (with Archit, Rafi, Justin, and Brian Stein present),
we decided that it is ok to suspend versioning for the duration
of the unpublished() method.
This puts the following items on my plate.
1. Fix the current implementation of suspendVersioning() such that
all pending events are flushed, before versioning is turned off.
2. Implement the resumeVersioning() method.
3. Remove the call to Versions.suspendVersioning() inside
the publish() method of ContentItem. Fix stuff that breaks
as a result.
4. Add a call to Versions.resumeVersioning() on exit of the
unpublish() method. Document the fact that versioning
is off for the duration of the unpublish() method.
Items 1 and 2 depend on BZ 100482.
I propose we punt on this one for RC1. Here are the reasons why.
At the time this bug was filed, I was not sure where the source of the
problem was. It could have been that versioning was not recording the
versioning log correctly. Or it could've been that some properties of
the ContentItem object type needed to be marked "unversioned".
Having looked into this, I am 99% certain that the versioning log is
recorded correctly. The problem is that the rollback functionality is
implemented in a less than robust fashion. Currently, this
incorrectness is only manifested, if I remove the call to
Versions.suspendVersioning() from the ContentItem.publish method. If
I leave things the way they currently are on the tip, then rollback
1. The discovered lack of robustness in the current implementation of
rollback does not affect anyone at the moment.
2. Once the bug is fixed, the change will not require data model
3. Fixing the current rollback implementation is going to take 2 to 3
In light of the above, I say we should defer this ticket until after
RC1. Sorry it took me so long to get to the bottom of this.
For the curious, here's an outline of what needs to happen for this
ticket to be fixed.
The current lack of rollback robustness is due to a combination of two
1. The initial implementation was coded up with the assumption that
the versioning log can be trusted to be "correct," i.e. all the
relevant events are expected to be there.
2. The current implementation assumes that all objects we are dealing
with are versioned ACSObjects.
Assumption 1 was useful at one point. It allowed me to add a lot of
assertions about the correctness of the versioning log. For example,
for any given data object, the sequence of CREATE and DELETE events
must alternate. You can't have two juxtaposed DELETE events - the
object cannot be deleted twice, without having been resurrected
sometime in between the two deletions. Having these assertions in
place helped me catch some implementation errors early on.
After the introduction of suspendVersioning/resumeVersioning, most of
these assertion are no longer valid. The versioning log *can* have
two consecutive DELETE events for the same data object, if the
versioning service was suspended at some point.
So, these assertions need to be either removed, or conditionalized on
whether or not we are running in the "strict" mode. (Having the
"strict" mode where all these assertions are enabled is useful for
The second factor is the current implementation assumes that it only
has to deal with "fully versioned" data objects. As of
the state transition diagram for the ProxyDataObject makes this
assumption. This assumption mostly holds true in the current system,
as a carry-over from the old days, when you had to derive everything
from VersionedACSObject in order to use the versioning service in a
semi-correct manner. Addressing factor #1 listed above would lead to
this faulty assumption being broken much more easily.
Fixing #2 requires supporting a more complicated state transition
diagram. Instead of the ASCII diagram in
we now have to support something along the lines of
which is a good deal more complicated.
Oh, and once I'm done with adding new states and state transitions,
the diffing code implementing the CMS history UI will need to be
revamped, too. Rafi wrote it (Justin might've had a hand in it, too),
and I haven't really looked at it yet.
Put on Troika final blockers list for tracking.
Moved to Rickshaw September
cleaning up stale bugs