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

Wed Sep 23 13:23:00 EDT 2009

In general though, the problem with the visitor pattern is that it  
does not work well for situations where you have an arbitrary (and  
dynamically changing) number of command types.  Visitors worked well  
for us for invocations since we have a small and finite set of API  
commands that do not change often.  I think it also works for  
configuration since again, there are a few high-level "classes" of  
features encapsulated by a ConfigurationBean impl.  So again, small  
and finite set, doesn't change often.  So within these constraints,  
there should be no need for weakly typed method invocations via  
reflection.  :)

My personal rule of thumb - if a reflection-based visitor approach is  
to be considered, then this is a bad application for the visitor  
pattern.

On 23 Sep 2009, at 18:17, Manik Surtani wrote:

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