[infinispan-dev] Configuration visitor - Re: [JBoss JIRA] Commented: (ISPN-145) No transport and singleton store enabled should not be allowed

[Manik Surtani]

On 10 Sep 2009, at 16:14, Vladimir Blagojevic wrote:

> Hi Galder, Mircea
>
> Ok I agree, lets make it static, not a problem. However, I do not  
> agree
> that we should just make ValidationVisitor. I'll make a generic  
> visitor
> and then a subclass ValidationVisitor.

+1.  Definite +1 to a strongly typed approach.

> There are many potential uses for
> visitor, I'll mention only the existing: implement applyOverrides in a
> much more meaningful, cleaner and faster algorithm and possibly inject
> as well.
>
> Cheers,
> Vladimir
>
> On 09-09-10 9:59 AM, Galder Zamarreno wrote:
>>
>> On 09/10/2009 11:38 AM, Vladimir Blagojevic wrote:
>>
>>> Hi Galder,
>>>
>>> Although I agree with you that reflection is little bit harder to
>>> understand the advantage of this visitor pattern is many fold.  
>>> Say, down
>>> the road, we have multiple of these visitors written, some of them,
>>> possibly, even written by third party clients.
>>>
>> Hmmm, can you be more specific? We're talking about here in the  
>> scope of
>> Configuration validation.
>>
>>
>>> Our entire configuration
>>> tree class hierarchy is statically or compile time tied tied to  
>>> these
>>> visitors and we don't want to go down this path. I'll give it  
>>> another
>>> look, we can go static but we need to be aware of the trade offs.
>>>
>> We've implemented the visitor pattern statically for invocation
>> execution (see org.infinispan.commands.Visitor. Mircea, you're the
>> original author so correct me if I'm wrong) and I don't we had any
>> problems dealing with extending it. In fact, I had to extend it  
>> myself
>> to cope with keySet and entrySet commands and wasn't that bigger deal
>> (https://jira.jboss.org/jira/browse/ISPN-94?page=com.atlassian.jira.plugin.ext.subversion%3Asubversion-commits-tabpanel 
>> )
>>
>>
>>> If I understand you correctly the approach you are suggesting is not
>>> modular. It is generally considered not to be a good practice to tie
>>> logical processing to data structures themselves when the  
>>> likelihood of
>>> extending logical processing is significant. The whole reason behind
>>> adopting visitor pattern on our configuration tree is the ability  
>>> to add
>>> new operations to existing configuration object structures without
>>> modifying these structures. By using visitor pattern and adhering  
>>> to the
>>> so called open/closed principle we allow room to do any kind of
>>> operations on our configuration object structures.
>>>
>> I see your point of not tying up the processing and the data  
>> structure.
>>
>>
>>> It might seems that
>>> validation is the last thing we are going to do - but it is not, for
>>> sure. And tomorrow when there is yet another operation we need to
>>> complete on configuration tree the implementation will be a breeze.
>>>
>> Hmmm, we could implement a swiss knife so that it can solve all the
>> problems in this world but before doing that, I'd like to hear more
>> specific examples ;). I mean, we could have a created a super generic
>> visitor pattern to deal with configuration validation, with
>> invocations...etc, but I'm against that since it's hard to read,  
>> hard to
>> follow and hard to debug. For reflection related nightmares, simply  
>> look
>> at JBC 1.x code.
>>
>> So, to sum up, I see the point of using the visitor pattern but let's
>> time things a little bit. We're dealing with configuration validation
>> here, so let's stick that for the moment. Also, let's go for a
>> statically typed one similar to the one already used for dealing with
>> invocations. I'd imagine that applied to this use case, this would be
>> like: visitSingletonStoreConfig, visitTransportType...etc.
>>
>> org.infinispan.config package:
>>
>> ValidationVisitor {
>>     void visitSingletonStoreConfig(SingletonStoreConfig ssc);
>>     void visitTransportType(TransportType);
>>     ....
>>     // instead of traversalCompleted, use more meaningfull name
>>     validate()
>> }
>>
>>
>> SingletonStoreConfig:
>>
>> public Object accept(Visitor visitor) throws Throwable {
>>     return visitor.visitSingletonStoreConfig(this);
>> }
>>
>>
>>> Cheers,
>>> Vladimir
>>>
>>> On 09-09-10 4:11 AM, Galder Zamarreno wrote:
>>>
>>>> Hi Vladimir,
>>>>
>>>> I had a look at the implementation and I'm not sure I understand  
>>>> the
>>>> need for the reflection visit calls in  
>>>> AbstractConfigurationBeanVisitor.
>>>> Using reflection makes harder to follow code and it's slower than  
>>>> typed
>>>> calls and I'm not sure of the reason to use it here.
>>>>
>>>> Also, I don't see the need for a standard  
>>>> ConfigurationValidatingVisitor
>>>> that does such validation. Instead, the way I see it working is
>>>> SingletonStoreConfig having some kind of callback method being  
>>>> called,
>>>> i.e. the traversalCompleted() call and within it,  
>>>> SingletonStoreConfig
>>>> can, using the ComponentRegistry, retrieve the Transport  
>>>> component and
>>>> see if it's set or not. Or alternatively, SingletonStoreConfig  
>>>> could use
>>>> the passed InfinispanConfiguration to do its validation.
>>>>
>>>> To sum up, I think each AbstractNamedCacheConfigurationBean
>>>> implementation should, if it requires to, have the ability to  
>>>> validate
>>>> the configuration via some kind of callback. Such callback should
>>>> probably is possibliy traversalCompleted() itself.
>>>>
>>>> WDYT?
>>>>
>>>>
>>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> infinispan-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Manik Surtani
manik at jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org