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

[WFCORE-531] Update subsystem registration to use a ModelVersion instead of the component parts of the version. #473

Merged
merged 2 commits into from Feb 6, 2015

Conversation

darranl
Copy link
Contributor

@darranl darranl commented Feb 5, 2015

No description provided.

@wildfly-ci
Copy link

Core - Full Integration Build 419 is now running using a merge of 2469401

@wildfly-ci
Copy link

Windows Build 841 is now running using a merge of 2469401

@wildfly-ci
Copy link

Linux Build 1145 is now running using a merge of 2469401

@wildfly-ci
Copy link

Windows Build 841 outcome was SUCCESS using a merge of 2469401
Summary: Tests passed: 2986, ignored: 95 Build time: 0:18:15

@wildfly-ci
Copy link

Linux Build 1145 outcome was SUCCESS using a merge of 2469401
Summary: Tests passed: 2986, ignored: 95 Build time: 0:18:25

@wildfly-ci
Copy link

Core - Full Integration Build 419 outcome was SUCCESS using a merge of 2469401
Summary: Tests passed: 2856, ignored: 378 Build time: 0:48:29

info.setMajorVersion(majorVersion);
info.setMinorVersion(minorVersion);
info.setMicroVersion(microVersion);
info.setVersion(version);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would throw an exception if version is null, and get rid of the null checks in the SubsystemInformationImpl getters (although I have some other conflicting suggestions there)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO that is established API that I don't want to be touching here. I just want to be ensuring I can use a ModelVersion not revisiting the mandating all subsytems have a version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the exisiting registerSubsystem() methods take one or more int parameters which cannot be null, so you end up with a version whether you like it or not

@bstansberry bstansberry added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Feb 6, 2015
@bstansberry
Copy link
Contributor

Looks good; I agree that changing the null semantics shouldn't be part of this work.

Probably shouldn't happen at all.

I believe the reason null is allowed is because ExtensionParsingContextImpl.setSubsystemXmlMapping can establish a SubsystemInformationImpl and it's only later when ExtensionContextImpl.registerSubsystem gets called that the version gets set. So there's a tiny window where it is null. And we expose this data structure via the management API, so it's possible a read of the data can happen during that window.

kabir added a commit that referenced this pull request Feb 6, 2015
[WFCORE-531] Update subsystem registration to use a ModelVersion instead of the component parts of the version.
@kabir kabir merged commit 9295c04 into wildfly:master Feb 6, 2015
@darranl darranl deleted the WFCORE-531 branch February 6, 2015 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
5 participants