On 18/03/2010, at 12:36 PM, Adam Heath wrote: > Scott Gray wrote: >> On 18/03/2010, at 11:48 AM, Adam Heath wrote: >> >>> Scott Gray wrote: >>>> On 18/03/2010, at 11:04 AM, Adam Heath wrote: >>>> >>>>> Scott Gray wrote: >>>>>> On 18/03/2010, at 10:50 AM, Adam Heath wrote: >>>>>> >>>>>>> Jacopo Cappellato wrote: >>>>>>>> Adam, >>>>>>>> >>>>>>>> this is going to be a long thread and I don't want it to be. >>>>>>>> I already tried to explain why I decided to change the primary key of >>>>>>>> OrderShipment without following the deprecation mechanism (that I >>>>>>>> think it is a very good best practice) in this specific situation. >>>>>>>> I perfectly know when and how the shipment plans (i.e. the feature for >>>>>>>> which the OrderShipment entity was introduced) were implemented: I >>>>>>>> know because I contributed it (and the OrderShipment entity of course). >>>>>>>> I also know that it was only partially implemented and so I would be >>>>>>>> very surprised (especially because no one over the years ever asked or >>>>>>>> reported bugs etc.. about it - apart from me that was aware of its >>>>>>>> existence) if it is really used in production by someone. This is >>>>>>>> simply to explain that I was not irresponsible when I did it. >>>>>>>> >>>>>>>> However you are right if you say that I can't be completely sure if >>>>>>>> someone somewhere is using the entity. Also, lately the community >>>>>>>> seems to be more inclined to transform "best practices" (to be applied >>>>>>>> "cum granu salis") into "policies" to be applied blindly. >>>>>>>> In some way this is good because it requires less skills and >>>>>>>> creativity (but more pragmatism) and also less skilled persons can >>>>>>>> become good reviewers just by following written rules. >>>>>>>> This is probably less interesting but maybe necessary given the grow >>>>>>>> of the community. >>>>>>>> >>>>>>>> So ok, I am convinced now, we have to deprecate that entity etc. >>>>>>>> >>>>>>>> What does your book of policies says about this: is it something I >>>>>>>> *have* to do as a punishment for this bad commit or will you take care >>>>>>>> of it? >>>>>>> If I implied that it had to be you to do it, I apoligize. I didn't >>>>>>> mean it to sound that way. >>>>>>> >>>>>>> I was trying to discuss with you about the reasons behind this change, >>>>>>> so that we could all agree as to what the reasons and use cases were. >>>>>>> >>>>>>> Now that it seems we are on the same page, we can go about fixing this >>>>>>> as a team. I'll admit, however, that I have never gone thru an entity >>>>>>> deprecation exercise. And OrderShipment is a very low-level entity, >>>>>>> that has tons of references, so doing this right could be very >>>>>>> complex. The reason why I was so heated was that I understood this >>>>>>> would be a lot of work, and I was trying to see if there is a better >>>>>>> way than changing the primary key. >>>>>>> >>>>>>> The reason the original committer is generally the best one to fix >>>>>>> their original commit, is because that original committer knows the >>>>>>> most about what they were trying to do. That original person knows >>>>>>> their own code they wrote, and probably has an understand of the rest >>>>>>> of the files that they were modifying. >>>>>>> >>>>>>> However, due to the scope of this deprecation, we need the rest of the >>>>>>> community to help out. Unless we decide to revert this primary key >>>>>>> change, and due this a different way. >>>>>>> >>>>>>>> PS: BTW, if no one will implement the deprecation pattern in the next >>>>>>>> few days I will do that, no problem. >>>>>>> There's no big hurry. My previously mentioned importer already had >>>>>>> the shipGroupSeqId available(it was hard-coded to 00001, as it's >>>>>>> somewhat simple), so it wasn't a big change for me personally. >>>>>> I'm yet to form an opinion on whether the approach Jacopo took was a >>>>>> valid one, I've been looking at the code and I don't yet understand >>>>>> what problems this change may have caused for downstream users. >>>>>> Adam, would you mind explaining in more detail how the change has >>>>>> effected you negatively? >>>>> A new field was added to the OrderShipment entity, and that field was >>>>> made a primary key. >>>>> >>>>> My importer, which started from an empty database, thru an error, >>>>> because this field was not set, and null is not allowed. As I said, >>>>> the fix for me was simple, as this particular job hasn't gone live >>>>> yet, so I don't have to worry so much about migration. >>>> I would argue that in this case the early failure was a good thing. If >>>> the deprecation pattern had been followed it's possible you wouldn't have >>>> noticed until much later and then would have had to deal with updating >>>> your importer as well as migrating the data from the old table to the new. >>> You don't understand how deprecation works. >> >> That is a strong statement but the teacher has spoken, I shall go and face >> the corner now. >> >>> The entity is renamed. So there would still be the failure in my >>> importer, as it tried to update an entity that doesn't exist. >> >> I assumed your importer was acting on the database directly, my bad. >> >>> Additionally, if someone upgrading their data does an xml dump, the >>> old data will be dumped with the entity named OrderShipment. After >>> installing the newer ofbiz, that identiy will also not exist, so the >>> xml import will fail as well, when it can't find the OrderShipment >>> entity(which was renamed to OldOrderShipment). >> >> >> Since I don't understand how deprecation works I guess I'm also failing to >> understand how what you describe would be better than what would happen >> right now. >> >> Once again, I'm not against the deprecation pattern, I'm just struggling to >> see how it would have resulted in something better for the downstream user >> (but because I don't understand how deprecation works this probably requires >> more explaining that it would for someone who is more enlightened). > > Read this: > http://cwiki.apache.org/confluence/display/OFBTECH/General+Entity+Overview > > the part about deprecated entities. > > Before deprecation: > > <OrderShipment />, has a table named "order_shipment" in the database. > > After deprecation: > > <OldOrderShipment tableName="order_shipment"/> > <OrderShip/> > <service name="convertOldOrderShipmentToOrderShip"/> > <!-- all other places that reference/deal with OrderShipment are > changed to deal with OrderShip --> > > > During upgrade, the new OrderShip entity will have a table created in > the database, called "order_ship". This table will be empty at this > point. > > The existing tables will not be changed at all. This is because the > OldOrderShipment entity is mapped to the "order_shipment". Any other > tables that also still exist and reference "order_shipment" in the > database will continue to function correctly. > > > Actually, I just noticed a problem with the deprecation policy. Any > existing entities that currently reference OrderShipment, by way of > <relation>, can't have those relations dropped. Those should be > changed to "OldFooRelation", and the fk-name set appropriately, so > that the referential links in the database will be kept and not destroyed.
I am aware of the document and understand the concepts put forward. What I am asking you is, how in this case would following the deprecation policy have resulted in a better downstream user experience? My current understanding was that Jacopo chose the approach he did because it was a simpler one and resulted in a cleaner data model (i.e. no Old* entity left lying around until the end of time). What I don't understand is how downstream users will suffer extra pain because the deprecation guideline wasn't followed in this case.
smime.p7s
Description: S/MIME cryptographic signature
