Red Hat Bugzilla – Bug 100484
do not suspend versioning in the ContentItem's publish() method
Last modified: 2007-04-18 12:55:58 EDT
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 works. So, 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 upgrades. 3. Fixing the current rollback implementation is going to take 2 to 3 days. 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 factors: 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 unit tests.) The second factor is the current implementation assumes that it only has to deal with "fully versioned" data objects. As of //core-platform/dev/src/com/arsdigita/versioning/ProxyDataObject.java#11, 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 //core-platform/dev/src/com/arsdigita/versioning/ProxyDataObject.java#11, we now have to support something along the lines of //core-platform/dev/src/com/arsdigita/versioning/doc-files/ProxyDataObject-1.png#3, 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