12 Replies Latest reply on Feb 9, 2011 11:16 AM by vblagojevic

    Make Infinispan GlobalConfiguration more DI friendly...

    mpokorny

      The purpose of this post is start some discussion, maybe my ideas are completely wrong or perhaps something interesting might be said in reply.

       

      All comments from me are regarding Infinispan v4.x.

       

      Currently there are several weaknesses in the way one configures a GlobalConfiguration, its just not possible set an instance directly once must set classnames & properties or some combo.

       

       

      • setAsyncListenerExecutorFactoryClass      
      • setAsyncTransportExecutorFactoryClass
      • setTransportClass
      • setTransportProperties
      • setReplicationQueueScheduledExecutorFactoryClass
      • setEvictionScheduledExecutorFactoryClass
      • setEvictionScheduledExecutorProperties
        • 1. Make Infinispan GlobalConfiguration more DI friendly...
          vblagojevic

          Hi,

           

          I do not understand what you mean. Can you include a code example?

           

          Vladimir

          • 2. Re: Make Infinispan GlobalConfiguration more DI friendly...
            mpokorny

            One can only pass in a class name and some properties - never a real instance for any of the properties listed above. This means imposes some real limitations for users of Infinispan.

            • It is a kludge to pass instances of objects as parameters to a custom class you may wish Infinispan to use, because you can only give a class name and sometimes String parameters. Sticking stuff in a threadlocal for later reading when Infinispan eventually creates your class is a ugly.
            • One can never control the lifecycle of the "thing" given to Infinispan because the outside world never gets an instance of it.
            • Decoration is impossible - probably a who cares for now but still it shows the limitations

             

            Think how much simpler Infinispan would be if it just had setters to take instances rather than class names. The guys could easily cut Configuration in half to become essentially a bean with setters and a few other small bits leaving them to do the cool stuff inside Infinispan.

             

            A real example that comes to heart is Hibernate's Configuration.

            • Everything is a String which sucks - no type safety at all or early parameter checking.
            • It is possible to set properties that dont make sense, eg one can set properties for both inbuilt pools
            • Rather than setting batchsize, autocommit and friends and other non String property types as Strings one could more easily validate at an earlier stage.
            • Again this would make Hibernate's Connection that much smaller.
            • No more Environment and only nice type safe setters for stuff like: NamingStrategy, TransactionManagerLookup, TransactionFactory, CacheProvider etc.
            • 3. Make Infinispan GlobalConfiguration more DI friendly...
              vblagojevic

              Give us a code example of how would you make GlobalConfiguration more user friendly.

              • 4. Make Infinispan GlobalConfiguration more DI friendly...
                vblagojevic

                Ok, a colleague from work clarified what you mean because he was thinking along the same lines. What are the configuration settings that you would like to see configured with instances directly?

                • 5. Re: Make Infinispan GlobalConfiguration more DI friendly...
                  mpokorny

                  I dont have a great understanding of all the core interfaces which are exposed on GlobalConfiguration, but taking a look at the setters its easy to see the groups of setters which contribute to the creation of some key interface. Listed below are 18 setters which most probably create 6 instances.

                   

                  setAsyncListenerExecutorFactoryClass(String)

                  setAsyncListenerExecutorProperties(String)

                  setAsyncListenerExecutorProperties(Properties)

                   

                  setAsyncTransportExecutorFactoryClass(String)

                  setAsyncTransportExecutorProperties(String)

                  setAsyncTransportExecutorProperties(Properties)

                   

                  setEvictionScheduledExecutorFactoryClass(String)

                  setEvictionScheduledExecutorProperties(String)

                  setEvictionScheduledExecutorProperties(Properties)

                   

                  setMarshallerClass(String)

                  setMarshallVersion(String)

                  setMarshallVersion(short)

                   

                  setReplicationQueueScheduledExecutorFactoryClass(String)

                  setReplicationQueueScheduledExecutorProperties(String)

                  setReplicationQueueScheduledExecutorProperties(Properties)

                   

                  One could simply add setters for each of the above "groups" of setters. Personally less is more, GlobalConfiguration has way too many methods, replacing those 18 methods w/ just 6 instance setters would at the very least make it simpler and less busy. If you guys really want to keep the pattern, perhaps create separate builders for each instance that is trying to be built.

                   

                  class MarshallerBuilder {

                     setType

                     setVersion(String);

                     setVersion(short) <--- ???

                  }

                   

                  etc

                   

                  I only mentioned Hibernate in a previous post as the Configuration class w/ lots of setters that take strings thing seems to be a pattern that JBoss has adopted as the means to configure the "key" entity in that product.

                  • 6. Re: Make Infinispan GlobalConfiguration more DI friendly...
                    manik

                    Good suggestions.  This way you could do something like:

                     

                     

                    GlobalConfiguration {
                    
                      MarshallerBuilder configureMarshaller();
                    
                    }
                    
                    MarshallerBuilder {
                    
                      MarshallerBuilder setInstance(Marshaller m);
                    
                      MarshallerBuilder setType(Class<? extends Marshaller> c);
                    
                      MarshallerBuilder setVersion(String s);
                    
                      MarshallerBuilder addProperty(String name, String value);
                    
                      MarshallerBuilder addProperties(Properties p);
                    
                    }
                    

                     

                    This will allow for a fluent API in configuring stuff as well:

                     

                    globalConf.configureMarshaller().setType(MyMarshaller.class).addProperty("foo", "bar").setVersion("5.0");
                    

                     

                    Also, related, all other setters in the GlobalConfiguration and even Configuration should return the Configuration or GlobalConfiguration instance for more fluent configuration.  E.g., it would be nice to be able to:

                     

                    config.setLockAcquisitionTimeout(1000).setUseLockStriping(true).setCacheMode("LOCAL");
                    

                     

                    Another area of complex programmatic configuration which could greatly benefit from this style of configuration is cache stores.

                    • 7. Make Infinispan GlobalConfiguration more DI friendly...
                      vblagojevic

                      This feature should be completed fairly soon. Thanks for your input Miroslav. Much appreciated.

                      https://issues.jboss.org/browse/ISPN-911

                      • 8. Make Infinispan GlobalConfiguration more DI friendly...
                        mircea.markus

                        This look so much better!

                        In your example, what about moving the setInstance(Marshaller m) method out of MarshallerBuilder and directly under GlobalConfiguration. I think it would be simpler to configure the GlobalConfiguration that way, especially through DI.

                        • 9. Re: Make Infinispan GlobalConfiguration more DI friendly...
                          mpokorny

                          @Manik,

                           

                          I think your trying to put too much on GlobalConfiguration by adding methods like "configureXXXBuilder" simply because today there is one real implementation of Marshaller in this case but tomorrow there could very well be more. When more builders become available it is not practical to keep adding more and more of them to GlobalConfiguration as it would suffer from too many methods once more in the form of builder factories.

                           

                          Rather than adding builder factories on GC i would create a new class and put all builders for the same end product type on it. It might look something like this:

                           

                          final public Marshallers {

                              public static LessTypeSafeMarshallerBuilder defaultFromStrings();

                              public static MoreTypeSafeMarshallerBuilder defaultBuilder();

                           

                              public static JBossMarshallerBuilder jbossMarshalling();

                              public static JdkMarshallerBuilder jdk();

                              public static ProtocolBufferMarshaller protocolBuffers(); etc

                           

                             private Marshallers(){

                                throw new UOE();

                            }

                          }

                           

                          Repeat this sort of pattern for all the other key interfaces, each gets its own static factory method only. Also GlobalConfiguration already has a lot of methods theres no need to put builder factories on it. I am not exactly sure if the original setters for Marshaller in this case remain... if so does the configureMarshaller() returned come preset w/ any preceeding setters or does it start off empty ? as its not clear if its an instance method on GC.

                           

                          If you want to keep the old String setters to build up something i might create two builders that both builder something like a Marshaller. THe non String setters builder would have only nice type safe getters. As an implementation detail the former builder (#1) would wrap the latter builder (#2) which validates and does the actual building. This means devs get a choice of 2 builders rather than one builder where it may appear to have duplicate almost overloading methods.

                           

                          LessTypeSafeMarshallerBuilder {

                             LessTypeMarhsallerBuilder setTypeName( String class );

                             LessTypeMarshallerBuilder setVersion(short );

                             LessTypeMarshallerBuilder set( String propertyName, String propertyValue );

                             Marshaller build();

                          }

                           

                          Anyone reading from a properties file would use LTSMB only because of set( String propertyName, String propertyValue ) etc.

                           

                          MoreTypeSafeMarshallerBuilder {

                            MoreTypeSafeMarshallerBuilder setXXX();

                            Marshaller build(); 

                          }

                           

                          Note the names are temporary and just named like so to make a clear differentiation.

                           

                          @Mircea

                           

                          Yes great summary of what i just typed above.

                           

                          hth

                          • 10. Re: Make Infinispan GlobalConfiguration more DI friendly...
                            mircea.markus

                            The way I understand it is that Manik's MarshallerBuilder would build all possible implementations of Marshaller interface, so you won't need a JBossMarshallerBuilder, JdkMarshallerBuilder etc but only used the base one. It is generic enough through setProperty method

                            • 11. Re: Make Infinispan GlobalConfiguration more DI friendly...
                              mpokorny

                              Of course but what if Infinispan ships more than MarshallerBuilder in the future. MarshallerBuilder is prolly the wrong interface to use but one of the other interfaces most likely will need more than one buidler given more than built in impl is available.

                               

                              With the magic setProperty(String,String) type safety is absent. What happened to better type safe setters ?

                              • 12. Make Infinispan GlobalConfiguration more DI friendly...
                                vblagojevic

                                I have finished initial change of GlobalCofiguration and I want to solicit some early feedback before I proceed further with Configuration class. We'll do together one final review before this feature request is integrated in 5.0 branch. We decided not remove current setters and getters, setters will be deprecated in 5.0 and removed in 6.0. It will be rather easy to configure these logical section of configuration using "builders" since all methods will use naming patters. In case if nested configuration elements parent builder will provide access to child builder (see SerializationBuilder in commit below).

                                 

                                GlobalConfiguration will expose builders like this:

                                 

                                GlobalConfiguration {

                                 

                                   ...

                                   public GlobalJmxStatisticsBuilder configureGlobalJmxStatistics();

                                   public SerializationBuilder configureSerialization();

                                   public TransportBuilder configureTransport(); 

                                   ...

                                 

                                }

                                 

                                And builder API in action looks like this:

                                 

                                public void testGlobalConfigurationBuilders() {

                                      GlobalConfiguration gc = new GlobalConfiguration();

                                      GlobalJmxStatisticsBuilder jmxStatistics = gc.configureGlobalJmxStatistics();

                                      jmxStatistics.setAllowDuplicateDomains(true).setEnabled(true);

                                     

                                      TransportBuilder transport = gc.configureTransport();

                                      transport.setClusterName("blah").setMachineId("id").setRackId("rack").setStrictPeerToPeer(true);

                                     

                                      ExecutorFactoryBuilder<ExecutorFactory> asyncTransportExecutor = gc.configureAsyncTransportExecutor();     

                                      asyncTransportExecutor.setClass(DefaultExecutorFactory.class).addProperty("blah", "blah");

                                           

                                      assert gc.isAllowDuplicateDomains();

                                      assert gc.isExposeGlobalJmxStatistics();

                                    

                                      assert gc.getMachineId().equals("id");

                                      assert gc.getRackId().equals("rack");

                                      assert gc.getClusterName().equals("blah");

                                     

                                      assert gc.getAsyncTransportExecutorFactoryClass().equals(DefaultExecutorFactory.class.getName());    

                                   }

                                 

                                My work branch is https://github.com/vblagoje/infinispan/commit/d99db029

                                Feedback appreciated.

                                 

                                Regards,

                                Vladimir