Re: [wildfly-dev] Capabilities and Requirements - Common Definitions

On 05/22/2015 01:06 PM, Darran Lofthouse wrote: > > > On 22/05/15 19:00, David M. Lloyd wrote: >> On 05/22/2015 12:55 PM, Darran Lofthouse wrote: >>> >>> >>> On 22/05/15 17:25, Brian Stansberry wrote: >>>> On 5/22/15 9:21 AM, Darran Lofthouse wrote: >>>>> >>>>> >>>>> On 22/05/15 13:57, Brian Stansberry wrote: >>>>>> Thanks for raising this; it's a good set of questions. >>>>>> >>>>>> I'll be AFK for a week starting late today, so I'll start with point #2 >>>>>> even though that's backwards. It's just the one where I have a stronger >>>>>> opinion. >>>>>> >>>>>> The controller module should be about basic API for creating resources, >>>>>> attributes, handlers. Having other stuff like the actual >>>>>> ModelControllerImpl in there isn't pure, but I don't mind much. But then >>>>>> over time I and others have put some stuff in there basically just >>>>>> because both server and host-controller use it. We need to move away >>>>>> from that kind of thing, and not add more. >>>>> >>>>> I can move it to a different module but most likely I think that would >>>>> be a new module, 'server' could have been a candidate but that depends >>>>> on too many artifacts that will also need to reference these. >>>>> >>>>>> As for your question 1, I don't feel strongly at all, just have some >>>>>> thoughts. To initially use a capability, people are going to have to >>>>>> look up data. We're not going to stick all of these in controller, so >>>>>> that means there's going to be a search, and these kinds of classes are >>>>>> going to be dispersed. Trying to maintain some fairly centralized >>>>>> document somewhere would actually make that initial search easier. >>>>> >>>>> At the moment with this PR all I am proposing is a central location for >>>>> capabilities that we know will be widely used, in this case the server >>>>> and subsystems etc. are going to have hard coded dependencies on Elytron >>>>> APIs - what is not hard coded is how the implementations of those APIs >>>>> are actually made available. >>>>> >>>> >>>> Who is the consumer of these? A requirer only needs the String names, >>>> none of the "public static final RuntimeCapability" instances. >>> >>> The consumers will be: - >>> - Various resources in core. >>> - Subsystems in general. >>> - Deployments. >>> >>> One possible problem I am seeing with only using a String on the >>> consumer side is that there is no type associated even though the >>> dependency injection for dynamic capabilities at least is going to >>> depend on a mutually agreed type. >> >> I don't think that really makes a huge difference in this case. There >> will be a fairly small number of consumers, and type safety in this case >> is not going to be critical. And really I don't anticipate that type >> safety would catch any bugs that wouldn't be caught in any event by the >> simpler string-based system at the same time. >> >>>> The different providers of the capability can use the RuntimeCapability >>>> constants, but I think those should be separated somehow. They are >>>> really none of the business of the requirers. >>> >>> The providers will be: - >>> - The Elytron Subsystem (Optional) >>> - Any other subsytem that wants to provide one or more of these >>> capabilities. >>> >>> For separation we could have different classes, different modules or >>> even some factory class to convert from the name to the >>> RuntimeCapability instance. >> >> Too complex - I think we need to think minimally here. Keep the >> mechanism as simple as possible. > > Which part was too complex? The Factory? > > IMO I think go for one new module, 2 classes in this module, 1 class for > the String constants and 1 class for the statically defined > RuntimeCapability definitions. Having a module for every capability, which is what this implies, is too complex. We should have zero modules with zero classes and every consumer can have their own copy of the one string that they need. Many providers won't even need special classes, so setting this precedent doesn't really make any sense. >>> I think single new module with two classes may be the better compromise >>> for now otherwise we risk two new modules - each with a single class. ^^^ This right here is the warning sign. If we're creating four-class modules, let alone two- or one-class modules, we've made a wrong turn. More modules per anything is a massive and unnecessary increase in complexity and maintenance burden. >>> The main point being that WildFly Core is the single common dependency >>> for everything >> >> For the moment. Not forever though. > > In what way? Everything is depending on 'controller' as that is where > the notion of capabilities are defined - are we saying the core of core > can be split out even further? The core of core may not even continue to exist in its current form beyond WildFly 10 or 11 once we implement the new management code. >>>>> The Elytron project could not contain these definitions as that has no >>>>> dependencies on WildFly, the Elytron Subsystem can not contain this as >>>>> that is an optional module. >>>>> >>>>>> Using constants like in your PR is helpful for later maintenance. If >>>>>> some constant is deprecated, referrers will see that in their IDE. They >>>>>> can easily click to read javadoc and learn if the requirement's dynamic >>>>>> and the service type. >>>>>> >>>>>> The biggest concern I have is avoiding premature classloading links. >>>>>> This is important in the case of optional requirements. If a requirement >>>>>> is optional, it's critical that the declaration of the requirement in >>>>>> the code doesn't force loading of classes from the required capability's >>>>>> module. Otherwise that module is no longer optional and its harder to >>>>>> create a slim distribution. >>>>> >>>>> In this case the only classes being referenced are classes from the >>>>> Elytron module and that module is going to be mandatory within the >>>>> server. >>>>> >>>> >>>> Yep. This classloading thing is more of a general concern, not >>>> particularly relevant to this particular case. People tend to code >>>> things up by copying existing code though, so it's good to consider the >>>> general problem before establishing precedents. >>>> >>>>> Generally it sounds like 'controller' is a no-go so the format of any >>>>> constants in code will probably dictate where this lives. >>>>> >>>>> If we are only talking Strings I may be able to add it to the >>>>> 'wildfly-core-security-api' module although javadoc can not reference >>>>> any capability related classes so that may not be preferable - of I can >>>>> create a new module with a dependency on controller and everything else >>>>> that uses Elytron capabilities can then depend on this new artifact / >>>>> module. >>>>> >>>>>> On 5/22/15 5:31 AM, Darran Lofthouse wrote: >>>>>>> Following on from PR https://github.com/wildfly/wildfly-core/pull/757 >>>>>>> and my final comment in the PR we need to have a discussion on how we >>>>>>> coordinate capability and requirement definitions - especially where >>>>>>> multiple components need a common definition. >>>>>>> >>>>>>> The first option is to do it all by convention and have no shared >>>>>>> constants, the down side of this is we now need to document this and >>>>>>> keep the document maintained. A document would also make it hard in >>>>>>> the >>>>>>> future to flag certain capabilities as deprecated if preferred >>>>>>> alternatives are made available. >>>>>>> >>>>>>> The second option would be to just define the Strings somewhere and use >>>>>>> Javadoc to specify if the capability is dynamic and it's service type. >>>>>>> >>>>>>> The third option is defining the string and RuntimeCapability instances >>>>>>> in a central place so they can both be referenced as needed. >>>>>>> >>>>>>> Any other options? >>>>>>> >>>>>>> Where these live will be a second point to discuss but that is heavily >>>>>>> driven based on what we will share in the first place. >>>>>>> >>>>>>> Regards, >>>>>>> Darran Lofthouse. >>>>>>> _______________________________________________ >>>>>>> wildfly-dev mailing list >>>>>>> wildfly-dev(a)lists.jboss.org >>>>>>> https://lists.jboss.org/mailman/listinfo/wildfly-dev >>>>>>> >>>>>> >>>>>> >>>> >>>> >>> _______________________________________________ >>> wildfly-dev mailing list >>> wildfly-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/wildfly-dev >>> >> > _______________________________________________ > wildfly-dev mailing list > wildfly-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/wildfly-dev >