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
[WFLY-2640] Unable to add cached-connection-manager after removing it once #9206
Conversation
<xs:annotation> | ||
<xs:documentation> | ||
Cached connection manager settings for resource adapters. NOTE: This service is always available so there's no | ||
way to remove it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case why is it represented in the xsd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you can actually configure it. My suggestion would be to change minOccurs
to 1
, but since that requires a schema change I don't know if this issue is the best one to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bstansberry are you ok with this now?
Sorry for the delay in digging into this. And for the wall of text to follow. ;) First, this CCM stuff is an odd case so it's good to improve it along these lines. So no basic objection. But, FWIW the general condition described in WFLY-2640 can be found all over the place in WildFly and we shouldn't be jumping through hoops doing one-off fixes for them. IOW I'm not much bothered by people removing a resource, getting a reload-required as part of the response to that reload, and then having an issue adding it back when they don't do the reload first. Well, I am bothered but the general solution is to declare the resource to provides a capability and then the WFCORE-1106 magic will handle it for you. (Read that JIRA for details.) Ok, now about this specific PR. First, making the add handler private is fine. Invoking it after boot never worked. So there is no incompatible change involved there. And not worth it IMO to bump the subsystem API version to correct a bug. This alone solves the JIRA by making it illegal to do the add. Next, making the remove handler private is an incompatible API change. The remove handler did actually work, just in an odd way. What it did, in terms of its practical effect, was the same as if it had undefined all the attributes in the resource, except 'install' which it set to 'false', and then triggered reload-required. So, we should replace the handler for the 'remove' operation with one that specifically does that. This way after the user does the 'remove' but before the reload, the model reflects the actual configured state, as opposed to inaccurately pretending the config has no resource at all. If you don't want to support 'remove' going forward, add deprecation metadata to its definition so it logs the deprecation warning. And then we can remove it in some future release. I don't see any need to discuss this 'remove' behavior in the xsd documentation. The minOccurs should not be 1 in the xsd. The element isn't actually required, and it's presence or absence is actually meaningful. If present, the 'install' attribute in the resource is 'true', otherwise it is 'false'. |
@bstansberry Would this do? I tested it locally and the |
Windows Build 5529 outcome was FAILURE using a merge of bf084ad Failed tests
|
/** | ||
* @author <a href="ingo@redhat.com">Ingo Weiss</a> | ||
*/ | ||
class CachedConnectionManagerRemove extends AbstractRemoveStepHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably shouldn't be an extension of any standard remove step handler, as it's very custom. See the last commit on https://github.com/bstansberry/wildfly/commits/pullRequest9206 for how the handler should look. Sorry; I should have provided that right off. I wasn't thinking about how custom this is.
protected void performRuntime(OperationContext context, ModelNode operation, ModelNode model) throws OperationFailedException { | ||
|
||
// We need to undefine any parameters and then set install to false, basically resetting the CCM settings | ||
model.get(JcaCachedConnectionManagerDefinition.CcmParameters.DEBUG.getAttribute().getName()).set(ModelType.UNDEFINED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an FYI, this does not set the node to 'undefined', i.e. node.isDefined() would return 'true'. Rather, it sets the node value to the ModelType enum UNDEFINED.
A ModelNode can have a ModelType as its value. This is one of the things that allows us to describe the management API solely using ModelNode, e.g. we can describe the type of an attribute as ModelType.BOOLEAN etc.
If you want to make a node undefined, use either node.clear() or node.set(new ModelNode()).
@iweiss in case you've not seen Brian's comments |
@kabir It totally slipped. Sorry. I tried @bstansberry's change and it triggers a loop at |
70c0644
to
022d64a
Compare
@kabir This is done now but can only be merged WildFly upgrades WildFly-Core to 3.0.0.Alpha13 due to wildfly/wildfly-core#1953 |
Retest this please |
Issue: https://issues.jboss.org/browse/WFLY-2640