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

[Vladimir Blagojevic]

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. 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
>>      
>