WFLY-11117 Executing legacy operations in mod_cluster subsystem #11726
Conversation
|
@honza-kasik Do you want to pretest this? |
|
@rhusar Did a manual pretest and it seems to me, that issue is fixed. Thanks! |
| return translateProxyPath(context, context.getCurrentAddress().getParent()); | ||
| } | ||
|
|
||
| static PathAddress translateProxyPath(OperationContext context, PathAddress address) throws OperationFailedException { | ||
| Set<Resource.ResourceEntry> children = context.readResourceFromRoot(address).getChildren(ProxyConfigurationResourceDefinition.WILDCARD_PATH.getKey()); |
There was a problem hiding this comment.
This should be context.readResourceFromRoot(address, false).....
This isn't a flaw in this PR but might as well fix it. :)
There was a problem hiding this comment.
Fixed, thanks for spotting this.
| ServiceName serviceName = ProxyConfigurationResourceDefinition.Capability.SERVICE.getServiceName(proxyAddress); | ||
| return context.getServiceRegistry(false).getService(serviceName); | ||
| return new ActiveServiceSupplier<ModClusterServiceMBean>(context.getServiceRegistry(false), serviceName).get(); |
There was a problem hiding this comment.
For a lot of the uses of this method, this should be context.getServiceRegistry(true).
It doesn't need to be for the getService(context) != null checks, but for the other ones if the handler is changing any state it should be 'true'.
* <strong>Note:</strong> It is very important that the {@code modify} parameter accurately reflect whether the
* caller intends to make any modifications to any object reachable, directly or indirectly, from the returned
* {@link ServiceRegistry}. This includes modifying any {@link ServiceController},
* {@link org.jboss.msc.service.Service}, {@link org.jboss.msc.value.Value} or any object reachable from a value.
*
* @param modify {@code true} if the operation may be modifying any object reachable directly or indirectly from
* the returned {@link ServiceRegistry}, {@code false} otherwise
There was a problem hiding this comment.
Fixed (not sure what it is for though and I assume there would be other usages of this incorrectly passing false, assuming its modifications to the registry itself, not the object internal state).
| @@ -172,15 +171,15 @@ public ModelNode execute(ExpressionResolver expressionResolver, ModelNode operat | |||
| public ModelNode execute(ExpressionResolver expressionResolver, ModelNode operation, ModClusterServiceMBean service) { | |||
| boolean enabled = service.enable(); | |||
|
|
|||
| return new ModelNode().get(ModelDescriptionConstants.RESULT).set(enabled); | |||
| return new ModelNode(enabled); | |||
There was a problem hiding this comment.
Most of these enums don't override 'apply' and thus don't configure an OperationDescription that describe any reply at all. OTOH that maybe gives you freedom to change the undocumented reply type. OTOH, it's not a correct description.
There was a problem hiding this comment.
Yeah, that was not the intention. Opened https://issues.jboss.org/browse/WFLY-11246 to address these.
…t possible with configuration with just one proxy
…esource recursively
|
@bstansberry Please update your review, thanks! :-) |
|
@bstansberry Thanks! |
bstansberry
added
the
ready-for-merge
|
Sorry it took so long! |
LegacyMetricOperationsRegistration#translateProxyPath need not read resource recursively
LegacyMetricOperationsRegistration#translateProxyPath need not read resource recursively
Jira
https://issues.jboss.org/browse/WFLY-11117