David Van Couvering (JIRA) wrote: > >Hi, all. Here is the proposed patch that provides the framework for code >sharing. I was thinking folks could look at it and discuss, and then once >issues have (hopefully) been worked out, we can have a vote. > > > Thanks for posting this patch. It helped me better understand the framework being proposed.l I looked at this perspective from the impact to existing users and risk of regression due to client/server jar incompatibilities.
Comments on the framework I think that this framework seems perhaps workable for very stable and well established interfaces such as SanityManager. I think it might be good to have the initial code sharing patch be just stable interfaces like this and not include the message sharing which presents different issues than just the java code. As I mentioned before, I think for the messages this framework is not acceptable. New/changed messages should not cause shadowing and require hasFeature() branches. One of the other mechanisms discussed to separate the resources or load from multiple resources seems more appropriate. I did not look closely at the message implementation after I decided that this is not really acceptable. For evolving interfaces or volatile resources the framework creates risk because. 1) There is a fairly high chance of regression in compatibility because the coding requirements for compatibility are not that intuitive and are not caught by build or normal testing. Also regressions in this area by definition will take a long time to appear in normal operation. 2)Adding/changing new shared methods will mean adding a hasFeature() branch but the old code path has to stay in place for backward compatibility, making code messy over time and hard to cover. 3) The infrastructure itself won't get too much of a workout for a long time as it won't be needed until after 10.2 These risks I do not think are showstoppers, but I do think that care should be taken when migrating code to the common area to make sure it is well established and stabilized on either server or client before being moved. Patch comments. I think it would be good to include one specific example feature to the code that shows how the hasFeature() code branches work for folks adding features or messages and talk about it a bit in package.html. I think the @author tags are not supposed to be in the files from what I have heard on the list. I don't have any other specific implementation comments beyond what Dan and Deepa have mentioned already. Tests I don't see what suite the test was added to. While the tests seem to test some of the assumptions made by the framework they don't seem to test the framework itself or cover the new common code. Again I think a solid example (if only a contrived one), and a test path that goes through it would be better if possible. I seem to recall some sort of problem in the build if the package and directory names don't match as for sc.newClass etc. Maybe that is not the case for tests but I thought maybe I should mention it. Thanks Kathey
