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.
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.
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.
The main point being that WildFly Core is the single common dependency
for everything
> 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
>>>
>>
>>