WFLY-13063: adjust sizes of id cache and confirmation window #13350
Conversation
wildfly-ci
added
the
deps-ok
There was a problem hiding this comment.
@ehsavoie Please review/advise.
In general, changes to an attribute's default value will require:
- Incrementing the model version for the subsystem (this may not be necessary, if the version has already been changed since the last release)
- Registration of appropriate transformers so that the domain controller to transform the model to any host controllers running older versions of WildFly. See: https://github.com/wildfly/wildfly/blob/master/docs/src/main/asciidoc/_extending-wildfly/Domain_Mode_Subsystem_Transformers.adoc
To achieve this, you can utilize this AttributeConverter instance:
https://github.com/wildfly/wildfly-core/blob/master/controller/src/main/java/org/jboss/as/controller/transform/description/AttributeConverter.java#L97
With this in place, there should be no need to "fix" the model in the subsystem transformer tests.
There was a problem hiding this comment.
Since this is going to miss WildFly 20 you need to bump the schema version.You should rebase your work over ehsavoie@a6ebd35. You should manage the model transformation in MessagingTransformerRegistration and not use ModelFixer as @pferraro wrote.
|
Ok, I'll look into that. What's the difference between ModelFixer and Transformer? They seem to be doing the same thing. |
|
ModelFixer 'fix' the model for tests. |
* bump messaging schema to 11 * bump messaging model to 11 Jira: https://issues.redhat.com/browse/WFLY-13518
There was a problem hiding this comment.
@ehsavoie The comments I've made here are really more for you than for @michpetrov. The subsystem's management API is fragile because of the use of these values from Artemis.
There's benefit to using the Artemis values. Perhaps we can have a subsystem test that validates all the defaults against a static value? If Artemis changes one, the test would inform us and then you can decide what to do.
| @@ -105,7 +105,7 @@ | |||
| .build(); | |||
|
|
|||
| SimpleAttributeDefinition BRIDGE_CONFIRMATION_WINDOW_SIZE = create("confirmation-window-size", INT) | |||
| .setDefaultValue(new ModelNode(FileConfiguration.DEFAULT_CONFIRMATION_WINDOW_SIZE)) | |||
| .setDefaultValue(new ModelNode(ActiveMQDefaultConfiguration.getDefaultBridgeConfirmationWindowSize())) | |||
There was a problem hiding this comment.
Our default values need to be statically declared in our own code base, and not be dependent on external libraries which can change without us noticing.
That or we have to have some test that will notice if any of the values change so we can update the API version, do transformation etc.
The code you're changing already had this problem, so you're not introducing any new problem.
| @@ -699,7 +699,7 @@ | |||
| * @see ActiveMQDefaultConfiguration#getDefaultIdCacheSize | |||
| */ | |||
| public static final SimpleAttributeDefinition ID_CACHE_SIZE = create("id-cache-size", INT) | |||
| .setDefaultValue(new ModelNode(20000)) | |||
| .setDefaultValue(new ModelNode(ActiveMQDefaultConfiguration.getDefaultIdCacheSize())) | |||
There was a problem hiding this comment.
Here you are introducing a new problem by converting a static value to a changeable one.
|
@bstansberry I removed all the references to ActiveMQDefaultConfiguration but it looks like i missed some other Artemis related default configuration. I've created https://issues.redhat.com/browse/WFLY-13575 to track this |
|
I have refactored the PR. |
|
@ehsavoie gotcha, I've updated the file |
|
/retest |
|
Superseded by #14095 |
Issue: WFLY-13063
id-cache-sizevalue is the same, I just changed it not to be hard-coded on our endconfirmation-window-sizechanged from 1048576 to 10485760, as this seems to be intended default value as per ARTEMIS-1261