Re: [Freeipa-devel] Command instantiation
On 01/15/2013 02:39 PM, Simo Sorce wrote: On Tue, 2013-01-15 at 10:23 +0100, Petr Viktorin wrote: On 01/14/2013 08:45 PM, John Dennis wrote: On 01/14/2013 12:56 PM, Jan Cholasta wrote: On 14.1.2013 18:50, Petr Viktorin wrote: Ah, yes, you've discovered my ultimate goal: rewriting the whole framefork :) It would seem we share the same ultimate goal, sir! :-) Well it's reassuring I'm not alone in my frustration with elements of the framework. I thought it was just me :-) I have one other general complaint about the framework: Too much magic! What do I mean by magic? Things which spring into existence at run time for which there is no static definition. I've spent (wasted) significant amounts of time trying to figure out how something gets instantiated and initialized. These things don't exist in the static code, you can't search for them because they are synthetic. You can see these things being referenced but you'll never find a class definition or __init__() method or assignment to a specific object attribute. It's all very very clever but at the same time very obscure. If you just use the framework in a cookie-cutter fashion this has probably never bothered you, but if you have modify what the framework provides it can be difficult. But I don't want to carp on the framework too much without giving credit to Jason first. His mandate was to produce a pluggable framework that was robust, extensible, and supported easy plugin authorship. Jason was dedicated, almost maniacal in his attention to detail and best practices. He also had to design most of this himself (the rest of the team was heads down on other things at the time). It has mostly stood the test of time. It's pretty hard to anticipate the pain points, that's something only experience with system can give you down the road, which is where we find ourselves now. +1. It's easy to criticize in hindsight, and I have great respect for the framework and its author. Nevertheless, software grows over time and we need to balance bolting things on with improving the foundations, so that we don't get stuck in a maze of workarounds in a few years. Can someone summarize how big a change this would be ? I do understand the general discussion, but I have not been involved deeply enough in the framework code to tell. Also how much would this conflict with the proposed LDAP change ? No, the LDAP changes won't affect the framework nor the plugin code. Do we have a way to slowly change stuff or will it require big all-or-nothing changes ? As far as I can tell without trying it, the change wouldn't be too disruptive. Command is now immutable, so to current code it shouldn't matter if api.Command.xyz returns a "global" instance or a freshly created one. There will be some complexity to make the change only for Commands and not the other plugin types, but it shouldn't be a big change. The next step is to not lock the Command class. Again I think the biggest issue will be to disable the locking only for Commands and not other classes in the hierarchy. When that's done, we can gradually change individual commands to store data in the Command object. I'd do one or two to make sure the approach works, after that it would be a series of small, localized, low priority patches. The main benefit is that *future* code wouldn't have to resort to thread-local storage. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On Tue, 2013-01-15 at 10:23 +0100, Petr Viktorin wrote: > On 01/14/2013 08:45 PM, John Dennis wrote: > > On 01/14/2013 12:56 PM, Jan Cholasta wrote: > >> On 14.1.2013 18:50, Petr Viktorin wrote: > >>> Ah, yes, you've discovered my ultimate goal: rewriting the whole > >>> framefork :) > >> > >> It would seem we share the same ultimate goal, sir! :-) > > > > Well it's reassuring I'm not alone in my frustration with elements of > > the framework. I thought it was just me :-) > > > > I have one other general complaint about the framework: > > > > Too much magic! > > > > What do I mean by magic? Things which spring into existence at run time > > for which there is no static definition. I've spent (wasted) significant > > amounts of time trying to figure out how something gets instantiated and > > initialized. These things don't exist in the static code, you can't > > search for them because they are synthetic. You can see these things > > being referenced but you'll never find a class definition or __init__() > > method or assignment to a specific object attribute. It's all very very > > clever but at the same time very obscure. If you just use the framework > > in a cookie-cutter fashion this has probably never bothered you, but if > > you have modify what the framework provides it can be difficult. > > > > But I don't want to carp on the framework too much without giving credit > > to Jason first. His mandate was to produce a pluggable framework that > > was robust, extensible, and supported easy plugin authorship. Jason was > > dedicated, almost maniacal in his attention to detail and best > > practices. He also had to design most of this himself (the rest of the > > team was heads down on other things at the time). It has mostly stood > > the test of time. It's pretty hard to anticipate the pain points, that's > > something only experience with system can give you down the road, which > > is where we find ourselves now. > > > > +1. > It's easy to criticize in hindsight, and I have great respect for the > framework and its author. > Nevertheless, software grows over time and we need to balance bolting > things on with improving the foundations, so that we don't get stuck in > a maze of workarounds in a few years. Can someone summarize how big a change this would be ? I do understand the general discussion, but I have not been involved deeply enough in the framework code to tell. Also how much would this conflict with the proposed LDAP change ? Do we have a way to slowly change stuff or will it require big all-or-nothing changes ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 01/14/2013 08:45 PM, John Dennis wrote: On 01/14/2013 12:56 PM, Jan Cholasta wrote: On 14.1.2013 18:50, Petr Viktorin wrote: Ah, yes, you've discovered my ultimate goal: rewriting the whole framefork :) It would seem we share the same ultimate goal, sir! :-) Well it's reassuring I'm not alone in my frustration with elements of the framework. I thought it was just me :-) I have one other general complaint about the framework: Too much magic! What do I mean by magic? Things which spring into existence at run time for which there is no static definition. I've spent (wasted) significant amounts of time trying to figure out how something gets instantiated and initialized. These things don't exist in the static code, you can't search for them because they are synthetic. You can see these things being referenced but you'll never find a class definition or __init__() method or assignment to a specific object attribute. It's all very very clever but at the same time very obscure. If you just use the framework in a cookie-cutter fashion this has probably never bothered you, but if you have modify what the framework provides it can be difficult. But I don't want to carp on the framework too much without giving credit to Jason first. His mandate was to produce a pluggable framework that was robust, extensible, and supported easy plugin authorship. Jason was dedicated, almost maniacal in his attention to detail and best practices. He also had to design most of this himself (the rest of the team was heads down on other things at the time). It has mostly stood the test of time. It's pretty hard to anticipate the pain points, that's something only experience with system can give you down the road, which is where we find ourselves now. +1. It's easy to criticize in hindsight, and I have great respect for the framework and its author. Nevertheless, software grows over time and we need to balance bolting things on with improving the foundations, so that we don't get stuck in a maze of workarounds in a few years. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 01/14/2013 12:56 PM, Jan Cholasta wrote: On 14.1.2013 18:50, Petr Viktorin wrote: Ah, yes, you've discovered my ultimate goal: rewriting the whole framefork :) It would seem we share the same ultimate goal, sir! :-) Well it's reassuring I'm not alone in my frustration with elements of the framework. I thought it was just me :-) I have one other general complaint about the framework: Too much magic! What do I mean by magic? Things which spring into existence at run time for which there is no static definition. I've spent (wasted) significant amounts of time trying to figure out how something gets instantiated and initialized. These things don't exist in the static code, you can't search for them because they are synthetic. You can see these things being referenced but you'll never find a class definition or __init__() method or assignment to a specific object attribute. It's all very very clever but at the same time very obscure. If you just use the framework in a cookie-cutter fashion this has probably never bothered you, but if you have modify what the framework provides it can be difficult. But I don't want to carp on the framework too much without giving credit to Jason first. His mandate was to produce a pluggable framework that was robust, extensible, and supported easy plugin authorship. Jason was dedicated, almost maniacal in his attention to detail and best practices. He also had to design most of this himself (the rest of the team was heads down on other things at the time). It has mostly stood the test of time. It's pretty hard to anticipate the pain points, that's something only experience with system can give you down the road, which is where we find ourselves now. -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 01/14/2013 01:17 PM, Rob Crittenden wrote: Petr Viktorin wrote: As for clearing the state, you already can't rely on that: the batch plugin doesn't do it. Yes, speaking of bolted on, that defines the batch plugin pretty well. It should be fairly straightforward to clear the state between executions though (or at least parts of it, there may be some batch-specif cthings we'd want to maintain). I think the problem is there are different groups of data being maintained in the context and we don't separate them into their logical domains. There is connection information that persists across all commands in the batch. Then there is the per command information specific to each command in the batch. Each should have it's own context. But as Petr3 points out the per thread context state is kinda a junk box where we throw in a undisciplined manner all the things we can't fit into a best practices programming model. Some of this is the fault of the framework design and the priorities that drove it. But not all of this can be laid at the feet of our framework. Some of it is due to the inflexibility of the core Python modules we use and their poor design. A good example is the fact the request URL is unavailable when processing a HTTP response in one of the libraries we're using, thats just bad design and because we use that library we have to live with it, hence sticking the request URL into the thread local context. There are numerous example of things like this, most are expedient workarounds to bad code design (some under our control, some not). -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 01/14/2013 07:17 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 01/14/2013 05:48 PM, John Dennis wrote: On 01/14/2013 11:06 AM, Petr Viktorin wrote: IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? Just a few thoughts, no answers ... :-) I agree with you that using thread local context blocks to pass cooperating data is indicative of a design flaw elsewhere. See the discussion from a couple of days ago concerning api locking and making our system robust against foreign plugins. As I understand it that design prohibits modifying the singleton command objects. Under your proposal how would we maintain that protection? The fact the commands are singletons is less of an issue than the fact they lock themselves when the api is finalized. If the commands instead acted as a "factory" and produced new instances wouldn't they also have to observe the same locking rules thus defeating the value of instantiating copies of the command? Good point. Actually, making api a factory would eliminate the reason for locking Commands: the foreign plugin could do whatever it wanted to its instance of the Command, and all would be well unless it modifies the class itself (which is about as bad as using setattr). I still think this would invite people to do even more dangerous things, like changing the API on-the-fly. Then let them. We're all adults here. You can't stop people from writing bad plugins. You can't stop attackers from killing your machine if you let them run any code. What's the point? The current design invites *us* to do things like setattr and thread-local state, just to get out of the boundaries we've set for ourselves. And the boundaries we need will grow over time, see the discussion of bolted-on things below. Sharp knives are safer than dull ones. I think the entire design of the plugin system has at it's heart non-modifiable singleton command objects which does not carry state. Yes. I guess I'm just trying to find ways to get out of that trap... context is your state, right? It is sort of bolted on but it is per-request and guaranteed not to leak into anything else (except batch, as noted below). We have and support the batch plugin, so that guarantee isn't worth much. Thread-local state works for now but as more things are built on it, it'll become unmanageable. FWIW, I was never convinced the trade-off between protecting our API and being able to make smart coding choices (such as your suggestion) struck the right balance. Going back to one of your suggestions of passing a "context block" as a parameter. Our method signatures do not currently support that as you observe. But given the fact by conscious design a thread only executes one *top-level* command at a time and then clears it state the thread local context block is effectively playing the almost exactly same role as if it were passed as a parameter. Almost. But, lots of Commands call other Commands, and the caller needs to know the internals of the callee to make sure the context attributes don't clash. Not to mention the other things, such as connection backends, that use the thread-local object. It's fragile. No argument there. We could clean this up somewhat by imposing a namespace into variable naming. As for clearing the state, you already can't rely on that: the batch plugin doesn't do it. Yes, speaking of bolted on, that defines the batch plugin pretty well. It should be fairly straightforward to clear the state between executions though (or at least parts of it, there may be some batch-specif cthings we'd want to maintain). There are also conne
Re: [Freeipa-devel] Command instantiation
Petr Viktorin wrote: On 01/14/2013 05:48 PM, John Dennis wrote: On 01/14/2013 11:06 AM, Petr Viktorin wrote: IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? Just a few thoughts, no answers ... :-) I agree with you that using thread local context blocks to pass cooperating data is indicative of a design flaw elsewhere. See the discussion from a couple of days ago concerning api locking and making our system robust against foreign plugins. As I understand it that design prohibits modifying the singleton command objects. Under your proposal how would we maintain that protection? The fact the commands are singletons is less of an issue than the fact they lock themselves when the api is finalized. If the commands instead acted as a "factory" and produced new instances wouldn't they also have to observe the same locking rules thus defeating the value of instantiating copies of the command? Good point. Actually, making api a factory would eliminate the reason for locking Commands: the foreign plugin could do whatever it wanted to its instance of the Command, and all would be well unless it modifies the class itself (which is about as bad as using setattr). I still think this would invite people to do even more dangerous things, like changing the API on-the-fly. I think the entire design of the plugin system has at it's heart non-modifiable singleton command objects which does not carry state. Yes. I guess I'm just trying to find ways to get out of that trap... context is your state, right? It is sort of bolted on but it is per-request and guaranteed not to leak into anything else (except batch, as noted below). FWIW, I was never convinced the trade-off between protecting our API and being able to make smart coding choices (such as your suggestion) struck the right balance. Going back to one of your suggestions of passing a "context block" as a parameter. Our method signatures do not currently support that as you observe. But given the fact by conscious design a thread only executes one *top-level* command at a time and then clears it state the thread local context block is effectively playing the almost exactly same role as if it were passed as a parameter. Almost. But, lots of Commands call other Commands, and the caller needs to know the internals of the callee to make sure the context attributes don't clash. Not to mention the other things, such as connection backends, that use the thread-local object. It's fragile. No argument there. We could clean this up somewhat by imposing a namespace into variable naming. As for clearing the state, you already can't rely on that: the batch plugin doesn't do it. Yes, speaking of bolted on, that defines the batch plugin pretty well. It should be fairly straightforward to clear the state between executions though (or at least parts of it, there may be some batch-specif cthings we'd want to maintain). rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 14.1.2013 18:50, Petr Viktorin wrote: On 01/14/2013 06:31 PM, Alexander Bokovoy wrote: On Mon, 14 Jan 2013, Jan Cholasta wrote: On 14.1.2013 17:06, Petr Viktorin wrote: IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? You could extend that to other plugin types as well (e.g. having Object instances with access to a single object's params instead of passing data around in a dict would be superb), but I'm afraid this kind of change won't be easy to do now. Ah, yes, you've discovered my ultimate goal: rewriting the whole framefork :) It would seem we share the same ultimate goal, sir! :-) But, rewriting the whole framework at once is not realistic, so I'd like to do this in little steps. Starting with Commands. That should be possible. The framework was designed around singleton plugin objects right from the beginning. I personally think this design sucks, as every kind of entity in IPA is described by such an object (object classes are singleton objects, methods are singleton objects, etc.), instead of using appropriate Python primitives for the job (object classes should be Python classes, methods should be methods of these classes, etc.). One note here. Having method classes separated from the object classes allows to add new methods to existing objects without affecting those objects and without redefining object classes. Ability to write simple plugins to extend existing objects is very attractive. In case of methods being methods of Python object classes you'd lose this feature and in order to gain it back you would need to deal with some sort of method descriptors -- in many cases methods are in fact collection of actual procedures working together, for example, most of CRUD methods have pre/post modifier callbacks, spanned over elaborate Method inheritance tree and allow to amend actual execute code. Pulling this into a single Python method would require more work that would later grow into an imitation of a separate method class anyway. Well, that is your idea, there are more ways around this. It would certainly feel more natural to work with such objects in Python than what we do now (feels rather C-ish to me). The difference between functions, methods, and callable objects in Python is surprisingly small. I'd bet allowing the implementer to use the best tool out out of those for her job would even involve less magic than the current approach. (We actually parse the class names with a regex!) +1 But as I said, this is generalizing my idea too much. I'd like to keep my feet on the ground so I'm currently proposing the change for Commands only. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 01/14/2013 06:31 PM, Alexander Bokovoy wrote: On Mon, 14 Jan 2013, Jan Cholasta wrote: On 14.1.2013 17:06, Petr Viktorin wrote: IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? You could extend that to other plugin types as well (e.g. having Object instances with access to a single object's params instead of passing data around in a dict would be superb), but I'm afraid this kind of change won't be easy to do now. Ah, yes, you've discovered my ultimate goal: rewriting the whole framefork :) But, rewriting the whole framework at once is not realistic, so I'd like to do this in little steps. Starting with Commands. That should be possible. The framework was designed around singleton plugin objects right from the beginning. I personally think this design sucks, as every kind of entity in IPA is described by such an object (object classes are singleton objects, methods are singleton objects, etc.), instead of using appropriate Python primitives for the job (object classes should be Python classes, methods should be methods of these classes, etc.). One note here. Having method classes separated from the object classes allows to add new methods to existing objects without affecting those objects and without redefining object classes. Ability to write simple plugins to extend existing objects is very attractive. In case of methods being methods of Python object classes you'd lose this feature and in order to gain it back you would need to deal with some sort of method descriptors -- in many cases methods are in fact collection of actual procedures working together, for example, most of CRUD methods have pre/post modifier callbacks, spanned over elaborate Method inheritance tree and allow to amend actual execute code. Pulling this into a single Python method would require more work that would later grow into an imitation of a separate method class anyway. The difference between functions, methods, and callable objects in Python is surprisingly small. I'd bet allowing the implementer to use the best tool out out of those for her job would even involve less magic than the current approach. (We actually parse the class names with a regex!) But as I said, this is generalizing my idea too much. I'd like to keep my feet on the ground so I'm currently proposing the change for Commands only. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On Mon, 14 Jan 2013, Jan Cholasta wrote: On 14.1.2013 17:06, Petr Viktorin wrote: IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? You could extend that to other plugin types as well (e.g. having Object instances with access to a single object's params instead of passing data around in a dict would be superb), but I'm afraid this kind of change won't be easy to do now. The framework was designed around singleton plugin objects right from the beginning. I personally think this design sucks, as every kind of entity in IPA is described by such an object (object classes are singleton objects, methods are singleton objects, etc.), instead of using appropriate Python primitives for the job (object classes should be Python classes, methods should be methods of these classes, etc.). One note here. Having method classes separated from the object classes allows to add new methods to existing objects without affecting those objects and without redefining object classes. Ability to write simple plugins to extend existing objects is very attractive. In case of methods being methods of Python object classes you'd lose this feature and in order to gain it back you would need to deal with some sort of method descriptors -- in many cases methods are in fact collection of actual procedures working together, for example, most of CRUD methods have pre/post modifier callbacks, spanned over elaborate Method inheritance tree and allow to amend actual execute code. Pulling this into a single Python method would require more work that would later grow into an imitation of a separate method class anyway. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 01/14/2013 05:48 PM, John Dennis wrote: On 01/14/2013 11:06 AM, Petr Viktorin wrote: IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? Just a few thoughts, no answers ... :-) I agree with you that using thread local context blocks to pass cooperating data is indicative of a design flaw elsewhere. See the discussion from a couple of days ago concerning api locking and making our system robust against foreign plugins. As I understand it that design prohibits modifying the singleton command objects. Under your proposal how would we maintain that protection? The fact the commands are singletons is less of an issue than the fact they lock themselves when the api is finalized. If the commands instead acted as a "factory" and produced new instances wouldn't they also have to observe the same locking rules thus defeating the value of instantiating copies of the command? Good point. Actually, making api a factory would eliminate the reason for locking Commands: the foreign plugin could do whatever it wanted to its instance of the Command, and all would be well unless it modifies the class itself (which is about as bad as using setattr). I think the entire design of the plugin system has at it's heart non-modifiable singleton command objects which does not carry state. Yes. I guess I'm just trying to find ways to get out of that trap... FWIW, I was never convinced the trade-off between protecting our API and being able to make smart coding choices (such as your suggestion) struck the right balance. Going back to one of your suggestions of passing a "context block" as a parameter. Our method signatures do not currently support that as you observe. But given the fact by conscious design a thread only executes one *top-level* command at a time and then clears it state the thread local context block is effectively playing the almost exactly same role as if it were passed as a parameter. Almost. But, lots of Commands call other Commands, and the caller needs to know the internals of the callee to make sure the context attributes don't clash. Not to mention the other things, such as connection backends, that use the thread-local object. It's fragile. As for clearing the state, you already can't rely on that: the batch plugin doesn't do it. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 14.1.2013 17:06, Petr Viktorin wrote: IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? You could extend that to other plugin types as well (e.g. having Object instances with access to a single object's params instead of passing data around in a dict would be superb), but I'm afraid this kind of change won't be easy to do now. The framework was designed around singleton plugin objects right from the beginning. I personally think this design sucks, as every kind of entity in IPA is described by such an object (object classes are singleton objects, methods are singleton objects, etc.), instead of using appropriate Python primitives for the job (object classes should be Python classes, methods should be methods of these classes, etc.). I really would like to see this improve, but I'm not sure if it's possible without rewriting the whole framework. Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Command instantiation
On 01/14/2013 11:06 AM, Petr Viktorin wrote: IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? Just a few thoughts, no answers ... :-) I agree with you that using thread local context blocks to pass cooperating data is indicative of a design flaw elsewhere. See the discussion from a couple of days ago concerning api locking and making our system robust against foreign plugins. As I understand it that design prohibits modifying the singleton command objects. Under your proposal how would we maintain that protection? The fact the commands are singletons is less of an issue than the fact they lock themselves when the api is finalized. If the commands instead acted as a "factory" and produced new instances wouldn't they also have to observe the same locking rules thus defeating the value of instantiating copies of the command? I think the entire design of the plugin system has at it's heart non-modifiable singleton command objects which does not carry state. FWIW, I was never convinced the trade-off between protecting our API and being able to make smart coding choices (such as your suggestion) struck the right balance. Going back to one of your suggestions of passing a "context block" as a parameter. Our method signatures do not currently support that as you observe. But given the fact by conscious design a thread only executes one *top-level* command at a time and then clears it state the thread local context block is effectively playing the almost exactly same role as if it were passed as a parameter. -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] Command instantiation
IPA Command objects sometimes need to pass some data between their various methods. Currently that's done using the thread-local context. For an example see dnsrecord_del, which sets a "del_all" flag in pre_callback and then checks it in execute. While that works for now, it's far from best practice. For example, if some Command can call another Command, we need to carefully check that the global data isn't overwritten. The other way data is passed around is arguments. The callback methods take a bunch of arguments that are the same for a particular Command invocation (ldap, entry_attrs, *keys, **options). By now, there's no hope of adding a new one, since all the callbacks would need to be rewritten. (Well, one could add an artificial option, but that's clearly not a good solution.) In OOP, this problem is usually solved by putting the data in an object and passing that around. Or better, putting it in the object the methods are called on. This got me thinking -- why do we not make a new Command instance for every command invocation? Currently Command objects are only created once and added to the api, so they can't be used to store per-command data. It seems that having `api.Command.user_add` create a new instance would work better for us. (Of course it's not the best syntax for creating a new object, but having to change all the calling code would be too disruptive). What do you think? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
