 |
History
| Log In
|
|
|
|
[
Permlink
| « Hide
]
Jerome Lacoste [21/Feb/05 02:58 AM]
That's the patch I sent him for testing. Martin said he would test it today or tomorrow.
I've done the watcher and reloader parts in
cruisecontrol_2.2.1_
I have to fix something in the patch. Will do it in the forthcoming days.
The last big thing remaining in that issue is to ensure that the newly added projects are correctly started. I am not sure what would be the best solution to fix that. My best options so far are:
1- call pause() then resume() around parseConfigFile() in parseConfigFileIfNecessary(). 2- only call resume() but modify resume so that it only starts project which are in stopped state. For that I can either add a isStopped() method to Project or expose the internal state 3- ??? Althought #1 is less invasive, my preference goes for #2. Feel free to let me how it should be done. I will implement the unit tests and the missing code when a decision is taken. I don't really like the idea of using a timer... seems arbitrary. plus, what happens if the project is currently building? what if it is queued?
I haven't thought of a good way to do it but I imagine triggering a config check off the BuildQueue, and perhaps update ProjectWrapper to check to ensure the project isn't stopped before calling execute. - my first implementation changed the BuildQueue & ProjectWrapper. I ended up kind of stuck and found that the code was too invasive to my taste. So I changed my approach and moved the reloading where it seemed the most coherent place: where the initial loading was already taken care of
- I tried to make the timer as less invasive as possible. That's why I put the implementation behind an interface. The idea would be to replace it if required. As you don't like the Timer it could be easily replaced by something that listens to another part of the system, in particular from the BuildQueue. It could even be configurable. - the fact that we currently accept external changes to the config, instead of having an API to make changes to the file make it harder to decide what to do when it comes to handle a change. By checking the file we are only notified of a coarse grained change: the whole config was changed in some way. Although it would be good to differentiate between project added/removed and project modified events, it is perhaps not a simple implementation to do. - if I recall, the projects reload their config just before getting build anyway. Based on that, my intent with the reload was to just detect newly added and removed projects. I don't replace existing projects by new obes if they have changed. I considered that modified projects were already handled by the internal reloading happening before the build is triggered. So the solution I have is not perfect. I don't remember the details now, but I now think that a changed schedule wouldn't affect the project until it is built... The fact that the persisted file is modified instead of the model makes it difficult to have a smart reload, especially as one part of the application already reloads itself. I'd rather keep things as simple as possible. Maybe we will come up with a client interface to make changes (e.g. the GUI) so that noone is obliged to go throught changing the config file by hand, thus solving this problem in a cleaner way? In the meantime I tried to keep the changes localized. I think we must decide first what we should do with the projects building status when we reload a change before we go on with the implementation. A: Should the modification of the config file be considered as a simple way to simulate a restart without restarting it for real, (i.e. we consider that pausing and queuing are not conserved)? B: or should we try to conserve the some sort of build status of the BuildQueue? If so what kind of meaning does this build status have compared to the rest of the queue. If the full config was changed, maybe it was done so that the full queue is recomputed? C: Should a removed/changed project currently built be stopped or should we let a project build finish? D: Should we try to identify exactly what changed? My answers to these questions were: A: we don't try to conserve pause and queing. B: we don't guarantee Build Queue conservation C: we let a build finish D: we don't try to identify what changed (althought it would be good) Comments appreciated. you raise some good points. here are my thoughts at the moment. (bais towards early rather than correct feedback here, so be kind.)
when the file is modified there are three things that can happen (thought more than 1 can happen at once): 1. change to existing project 2. project added 3. project removed if there was a change to an existing project we don't need to do anything. as you noted, the project are already reloaded as part of a build, so no worries there. (you are also correct that a change to the schedule won't go into effect until after that build, but that hasn't been a show stopper.) if a project is added it is pretty minimal impact. no worries about what is currently building or what is in the build queue. just need to create the Project, start it, and then step back and let it go. it is only if the project has been removed that we need to worry about doing anything complicated... now the removed project can be in one of four states: 1. paused 2. waiting for next build time 3. in build queue 4. building if the project is paused or waiting we should be able to stop it and that should be the end of that. (need to fix Project.stop() and perhaps Project.run() so that this is true as that wouldn't be the case now. a stopped project can be added to the build queue? that's just wrong...) if the project is building then I agree that we should just let it finish building. so the only real difficulty is what to do if the project is in the BuildQueue. I think we do not want the project to build, so the question is how to go about doing that. and now that I've been looking at the code for a couple of minutes I think I see an easy way: modify Project.execute() to check stopped on the same line it checks isPaused. that would prevent the project from building but requires no manipulation of the build queue. so I think changes to Project's stop(), run(), and execute() handles everything except for the actual reload of the config file... I think your idea of having the reload happen CruiseControlController is right, it is just a question of how it is triggered. I don't want BuildQueue to know about CruiseControlController but perhaps we could use an interface here so that th CCController can pass itself in when it creates the Queue and then the Queue will notify the interface (the Controller) before every build. that would give the controller the opportunity to check the file and start/stop any projects that need it. are there any issues w/adding and/or removing plugins from the root registry? I'm going to let Joris think about that as I'm getting too sleepy... :) so finally let me check what I said above against your 4 questions: A: I don't think of a modification as a restart. most modifications will be to existing projects and we don't really want to change the current behavior in that case. B: what I propose above doesn't require any change to the BuildQueue or ProjectWrapper, at least not as far as responding to projects being removed. projects being removed/stopped would be invisible to the Queue. C: removed Projects should be stopped -- by calling Project.stop() -- but that wouldn't intefere w/a build in progress. D: I don't think we need to indentify exactly what changed. Project added/removed seems fine. <quote>
are there any issues w/adding and/or removing plugins from the root registry? I'm going to let Joris think about that as I'm getting too sleepy... :) </quote> Yes, there are: currently, the root registry is loaded in a static initializer and never reread. I'll expose a static method to reload the root registry and check for other possible changes needed to deal with plugins when reloading the config file. I added a static method resetRootRegistry() to PluginRegistry, which clears all default properties registered with the root registry, clears the known plugins and rereads default-plugins.properties. This method should be called from the CruiseControlController before re-registering the global plugins defined in the config file when reloading the config file.
Some initial remarks after first review:
I'm not very good with multi-threading, but I don't think that you need to synchronize on an instance variable projectsLock in methods like resume; why not make the projectsMap variable a synchronized Map? Also, your copySet method is unneccesary: Set already has a method called addAll which does exactly the same thing. Also, I'm not sure what will happen if the Timer will run at the same a Project reloads its config. Did you test that? All in all, I think this implementation is heading in the right direction. I'll have to run some tests to get a better feel for the behaviour under exotic circumstances. Jerome, are you still working on this or should I take what's here and run with it?
I had planned to work on it this week-end. Feel free to do it if you want, just let me know.
I can wait the weekend. plenty of other stuff to work on... :)
As noted by "Comment by Jeffrey Fredrick [21/Apr/05 11:38 PM]" the only difficult part is how to remove a project. (cahnges are already taken care of by the projects, and adding projects should have no affect on other projects making the implementation rather trivial) However, in the original description, it says that already CC checks if the project has been removed and if so throws net.sourceforge.cruisecontrol.CruiseControlException: Project not found. Why not change this so that instead of throwing an exception, it removes itself somehow?
that is an interesting idea. perhaps we should introduce a new ProjectNotFoundException (extends CCException), throw that from ProjectXMLHelper in place of the current CCException, then handle that differently in Project.execute().
maybe rethrow it and handle it in ProjectWrapper such that getResult returned something, then let it bubble up to the controller via the BuildQueue? (my thinking here is to pass around the controller as little as possible) Handling removal that way mean that the getProjects() can return something somewhat inconsistent with what the config file can contain. That can be confusing to the users.
Am I the only one to think that having different cases (remove, update, add) handled by so different code paths is strange? Thanks Jerome for the patch. I'm currently working on integrating CC, CVS and Maven (root project.xml/maven.xml to "subclass" to get the CC/CVS/Maven integration in a snap without having to log into the CC host) and your contribution helped me a lot in moving forward.
I've fixed a few flow/synchronization bugs on your patch (and might have introduced more). See attachement (patch-ConfigHotRefresh.txt). It's nevertheless still a hack and this feature definitely deserves a clean refactoring of the synchronization mechanism. Once I'm done with the Maven side, I'll reopen my Java Threads book and replace those raw mutexes by smarter tools. Tangi 2 comments:
> if there was a change to an existing project we don't need to do anything. as you noted, > the project are already reloaded as part of a build, so no worries there. (you are also > correct that a change to the schedule won't go into effect until after that build, but > that hasn't been a show stopper.) It's not a priority right now, but in the future I think I will like this to be reconsidered. Something else I would like is to split the configuration in pieces. I would like the system to: - allow different users to modify different projects - not make the full system go down when one project's configuration is broken. I haven't looked into a design for this problem, but please try to keep in mind these ideas when addressing issues related to configuration management. commited.
used pieces of the patches but with some differences. most significantly I reparse based on MD5 instead of timestamp and I check for reparse when build queue gets a new project instead of using a timer. also added jmx method to reload the config. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
Handle removed task from cruisecontrol config file
