Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make custom SPI services more Spring friendly #6567

Closed
dsukhoroslov opened this issue Oct 26, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@dsukhoroslov
Copy link
Contributor

commented Oct 26, 2015

the current spring schema for service type is:

<xs:complexType name="service">
    <xs:sequence>
        <xs:element name="name" type="xs:string" minOccurs="1" maxOccurs="1"/>
        <xs:element name="class-name" type="xs:string" minOccurs="0" maxOccurs="1"/>
        <xs:element name="properties" type="properties" minOccurs="0" maxOccurs="1"/>
        <xs:element name="configuration" minOccurs="0" maxOccurs="1">
            <xs:complexType>
                <xs:sequence>
                    <xs:any minOccurs="0" maxOccurs="unbounded"/>
                </xs:sequence>
                <xs:attribute name="parser" type="xs:string" use="required"/>
            </xs:complexType>
        </xs:element>
    </xs:sequence>
    <xs:attribute name="enabled" type="xs:boolean" default="true"/>
</xs:complexType>

I'd suggest to add an ability to specify external spring bean as service implementation and change attribute type to string - in order to be able to specify attribute value as spring property (like ${my.service.enabled}). See the proposed schema. This change should be backward-compatible.

<xs:complexType name="service">
    <xs:sequence>
        <xs:element name="name" type="xs:string" minOccurs="1" maxOccurs="1"/>
        <xs:choice minOccurs="1" maxOccurs="1">
            <xs:element name="class-name" type="xs:string"/>
            <xs:element name="implementation" type="xs:string"/>
        </xs:choice>
        <xs:element name="properties" type="properties" minOccurs="0" maxOccurs="1"/>
        <xs:element name="configuration" minOccurs="0" maxOccurs="1">
            <xs:complexType>
                <xs:sequence>
                    <xs:any minOccurs="0" maxOccurs="unbounded"/>
                </xs:sequence>
                <xs:attribute name="parser" type="xs:string" use="required"/>
            </xs:complexType>
        </xs:element>
    </xs:sequence>
    <xs:attribute name="enabled" type="xs:string" default="true"/>
</xs:complexType>

Thanks, Denis

@emrahkocaman

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2015

Thanks for the advise @dsukhoroslov, I think this is doable for 3.7

@dbrimley

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

+1 for this feature, but on a wider scope.

Just to add my $0.5, wherever hazelcast constructs a class via the class-name property on a configuration element we should allow for an implementation property as well, which short circuits the internal object creation process.

For my own personal use case I wanted to inject a spring constructed ClientLoginModule. This ClientLoginModule had already been created with collaborators such as Spring LDAP.

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2016

I could implement it for 3.7. Any suggestions/objections?

@emrahkocaman

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

Hi @dsukhoroslov ,

I've no objection and contributions are, for sure, more than welcome.
Please feel free to share the progress and discuss ideas here or in https://gitter.im/hazelcast/hazelcast.

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2016

Regarding the wider scope mentioned by @dbrimley. It should be possible to specify base abstarct type with two optional attributes: "class-name" and "implementation", and then extend corresponding types/elements from this base type. But some elements do not follow this naming pattern (see queue-store, ssl, quorum types). If we'll change their definition to new schema this will make them non backward-compatible. Projects with these spring elements will have to make a small config changes when they'll move to HZ 3.7. Well, they'll do this any way, at least when will specify new HZ schema version. Are we ok with issue?

@emrahkocaman

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

Yes, this will break backward compatibility but I think this is acceptable to make all configs aligned.
@mesutcelik WDYT?

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2016

Please see the proposeed schema. Two new abstract types (implementable, configurable) were added at the very bottom of it.

hazelcast-spring-3.7.txt

@mesutcelik

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

Thanks @dsukhoroslov ,

This is rather big change. If we update naming in spring, we should update corresponding hazelcast-config entries. They are tightly coupled in terms of naming. I suggest adding only implementation where applicable into spring. Big refactoring and related discussion should be moved to a new github issues. That is what i think....

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2016

Hi @mesutcelik,
Actually, the quorum class is a bit different, so I left it as it is. Non-compatible changes were applied on queue-store and ssl types only. It is not a big deal for me to rename attribute names in the corresponding Spring configuration handling classes.
Or, I can drop the non-compatible changes and will go futher with backward-compatible only, up to you.

@mesutcelik

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

lets fix backward-compatibles only in the context of this issue...

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2016

ok, agreed

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2016

started to work on this and #7363. The following schema changes applied:

  • class-or-bean-name AttributeGroup introduced, element definitions who use class-name/implementation attributes generalized;
  • login-module.implementation handling added
  • service.implementation handling added
  • map.partition-strategy handling added
  • multicast.trusted-interfaces handling added

And the following missed attributes added to hazelcast-spring-3.7 schema:

  • multicast.loopbackModeEnabled
  • aws.iam-role
  • executor-service.statistics-enabled
  • queue.statistics-enabled
  • multimap.statistics-enabled
  • list.statistics-enabled
  • set.statistics-enabled

if you're ok with the changes I'll create PR.
Also, found a small bug in Map configuration processing at https://github.com/hazelcast/hazelcast/blob/master/hazelcast-spring/src/main/java/com/hazelcast/spring/HazelcastConfigBeanDefinitionParser.java#L641, it must be childNode at this line, I believe.

@emrahkocaman

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

Solved by #8202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.