Community
Participate
Working Groups
Build Identifier: WTP 3.2 In our WTP adaptor product we make use of the Publishers framework to allow Java code generators to execute while publishing the application to the server. As a result of these publishers generating new Java code they need to build the code before they terminate so that the auto-build functionality of Eclipse does not execute post-publish and cause a second publish to occur. The problem with this in WTP 3.2 is that the JDT builder (which compiles this newly generated code) wants to lock the entire workspace yet the server publish job only locks the server and published projects. As a result the project build will cause a rule conflict with the publish job. This was not a problem with prior releases of WTP since the server locked the entire workspace during a publish and was only exposed when using WTP 3.2. Reproducible: Always
Angel, please assign this one to me.
Created attachment 173877 [details] v1.0
Elson, what is the test coverage on this patch?
I review the code, a few comments: - For the logic in ServerBehaviourDelegate, it would be better if we don't always recalculate the publish cache, but if instead we check if the PublisherDelegate has changed the module, and if so then recalculate the publish cache. - I see there is a method inside PublisherDelegate that still says todo, please review. - There was white spaces that were added that should be removed Once my comments are address, don't forget to prepare this bug for PMC approval: http://wiki.eclipse.org/index.php/WTP_PMC_Defect_Review
Created attachment 174149 [details] v1.1 Updated patch with the review comments addressed.
* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. The publisher framework currently can cause the publisher fails to run when project specific lock is enabled. This is a regression on function with the new Publisher framework comparing to it's predecessor PublishTask framework in previous release. * Is there a work-around? If so, why do you believe the work-around is insufficient? This is no workaround to this problem. * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? 1. Testing a publisher that does not override the new methods. The publisher is running fine without a problem. 2. Testing another publisher that does override the new methods with changing the lock rule to lock entire workspace. The publisher runs fine. Also, tested the run on server scenario to make sure the changing lock rule does not cause * Give a brief technical overview. Who has reviewed this fix? The fix for this problem allows a publisher to specify the lock for the duration of the publisher. By default the lock will stay as the lock provided by the server unless the publisher delegate overrides the PublisherDelegate#getRule() method to return a different lock. Server adopter can use this API to set the lock to be the workspace lock so that publisher do not have any issue with builds causing lock conflicts. Adopter will also need to override the PublisherDelegate#isModifyModules() API to return 'true' so that the server tools cache is rebuild so that the publisher generated code does not cause a second publish. Angel has reviewed the fix and the updated patch contains changes as requested by the review. * What is the risk associated with this fix? Low for existing adopter who doesn't override the new methods. The behaviour should be the same as before. The risk is medium for adopters to override the new methods and take advantage on this fix since we are switching the locking rule of the publisher operation. Other jobs that are waiting for the lock can potentially be run while switching the lock rule on the job so the publish job will be delayed in those cases.
Elson, In the '* What is the risk associated with this fix' you mentioned that the risk is low for 'existing adopter who doesn't override the new methods.' But I think what you wanted to say was that the adopters of the current Publisher framework are not affected by the new methods because a default behaviour is given.
+1. Discussed with Elson and other job/locking experts at length. This patch does change the locking behaviour of publishing, and as such has a high risk. However, we've been through various scenarios and the only possible clients we could see "broken" by this are ones that have a fundamentally broken design to being with (and hence are likely already having problems). Elson, Angel, and others will be testing this thoroughly over the coming weeks to make sure there are no regressions.
changes committed to 32M
Changes released to 32M
Changes committed to HEAD
Changes released to HEAD
Updated the keyword to state that this defect has been PMC_approved after getting the two required PMC_approved votes
New Gerrit change created: https://git.eclipse.org/r/108972
