- Notifications
You must be signed in to change notification settings - Fork2.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
WFLY-20077: do not set default value of derived attributes, remove du…#18525
base:main
Are you sure you want to change the base?
Conversation
| @@ -192,13 +161,11 @@ static AddressSettings createDefaultAddressSettings() { | |||
| settings.setAutoCreateAddresses(AddressSettingDefinition.AUTO_CREATE_ADDRESSES.getDefaultValue().asBoolean()); | |||
| settings.setAutoDeleteAddresses(AddressSettingDefinition.AUTO_DELETE_ADDRESSES.getDefaultValue().asBoolean()); | |||
| settings.setAutoDeleteCreatedQueues(AddressSettingDefinition.AUTO_DELETE_CREATED_QUEUES.getDefaultValue().asBoolean()); | |||
| settings.setMaxReadPageBytes(AddressSettingDefinition.MAX_READ_PAGE_BYTES.getDefaultValue().asInt()); | |||
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.
Why is that removed ??
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.
@ehsavoie For the same reason as the other one, in this case the default formaxReadPageBytes is2 * pageSizeBytes
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.
It has been set to this value for backward compatible reasons due to changes in paging, so this value is expected
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.
It's the same issue. We can end up in a situation where we're ignoring user setting because our default for a MAX_VALUE is less than a user-defined BASE_VALUE.
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.
ok
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.
I agree
| settings.setDeadLetterAddress(asSimpleString(DEAD_LETTER_ADDRESS.getDefaultValue(), null)); | ||
| settings.setExpiryAddress(asSimpleString(EXPIRY_ADDRESS.getDefaultValue(), null)); | ||
| settings.setExpiryDelay(AddressSettingDefinition.EXPIRY_DELAY.getDefaultValue().asLong()); | ||
| settings.setDefaultLastValueQueue(AddressSettingDefinition.LAST_VALUE_QUEUE.getDefaultValue().asBoolean()); | ||
| settings.setMaxDeliveryAttempts(AddressSettingDefinition.MAX_DELIVERY_ATTEMPTS.getDefaultValue().asInt()); | ||
| settings.setMaxRedeliveryDelay(AddressSettingDefinition.MAX_REDELIVERY_DELAY.getDefaultValue().asLong()); |
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.
You should also remove the default value on the attribute definition
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.
I wanted to say that would require a schema bump but it looks like the default values are not in the schema to begin with. Still, the attribute does technically have a default value, it just isn't explicitly set, shouldn't we mention that in the description?
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.
Yes a version bump is required since we need transformation of the default values but the XSD can stay at its current version
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.
You need also to add transformers
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.
I don't follow. A transformer is for changing old attributes into new attributes, right? Since we don't use the default value anywhere we don't care if the old attributes have 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.
👍
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.
@ehsavoie I've updated the PR, I assume we want a test for this but is there a point in running all the other tests that are copied over from the 16.0 testcase?
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.
I don't think so
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.
@ehsavoie I'm confused about the tests, the basic test looks like this
public void testTransformersWildfly28() throws Exception { testTransformers(ModelTestControllerVersion.MASTER, MessagingExtension.VERSION_15_0_0);}so I thought I'd just copy it and run it forMessagingExtension.VERSION_16_0_0 but these test are being skipped ("No legacy controller to test against") and if I would force them to run they'd fail, since MASTER = "core version" and because Messaging is not in Core we cannot get the correct artifact onthis line so I don't know why these tests exist. And since those doesn't work I don't know how to test the default values.
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.
The version should be 17 not 16.1
@michpetrov@ehsavoie I'd like to see this one get to a mergable state, or, if there's no consensus on the fix, closed. |
| settings.setDeadLetterAddress(asSimpleString(DEAD_LETTER_ADDRESS.getDefaultValue(), null)); | ||
| settings.setExpiryAddress(asSimpleString(EXPIRY_ADDRESS.getDefaultValue(), null)); | ||
| settings.setExpiryDelay(AddressSettingDefinition.EXPIRY_DELAY.getDefaultValue().asLong()); | ||
| settings.setDefaultLastValueQueue(AddressSettingDefinition.LAST_VALUE_QUEUE.getDefaultValue().asBoolean()); | ||
| settings.setMaxDeliveryAttempts(AddressSettingDefinition.MAX_DELIVERY_ATTEMPTS.getDefaultValue().asInt()); | ||
| settings.setMaxRedeliveryDelay(AddressSettingDefinition.MAX_REDELIVERY_DELAY.getDefaultValue().asLong()); |
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.
The default values won't be used because if the value is not defined then it should be computed based on the other attribute value and not a default value
| settings.setDeadLetterAddress(asSimpleString(DEAD_LETTER_ADDRESS.getDefaultValue(), null)); | ||
| settings.setExpiryAddress(asSimpleString(EXPIRY_ADDRESS.getDefaultValue(), null)); | ||
| settings.setExpiryDelay(AddressSettingDefinition.EXPIRY_DELAY.getDefaultValue().asLong()); | ||
| settings.setDefaultLastValueQueue(AddressSettingDefinition.LAST_VALUE_QUEUE.getDefaultValue().asBoolean()); | ||
| settings.setMaxDeliveryAttempts(AddressSettingDefinition.MAX_DELIVERY_ATTEMPTS.getDefaultValue().asInt()); | ||
| settings.setMaxRedeliveryDelay(AddressSettingDefinition.MAX_REDELIVERY_DELAY.getDefaultValue().asLong()); |
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.
Thus we should convert the default value to a proper one in the transformer otherwise the buggy value is passed
| @@ -192,13 +161,11 @@ static AddressSettings createDefaultAddressSettings() { | |||
| settings.setAutoCreateAddresses(AddressSettingDefinition.AUTO_CREATE_ADDRESSES.getDefaultValue().asBoolean()); | |||
| settings.setAutoDeleteAddresses(AddressSettingDefinition.AUTO_DELETE_ADDRESSES.getDefaultValue().asBoolean()); | |||
| settings.setAutoDeleteCreatedQueues(AddressSettingDefinition.AUTO_DELETE_CREATED_QUEUES.getDefaultValue().asBoolean()); | |||
| settings.setMaxReadPageBytes(AddressSettingDefinition.MAX_READ_PAGE_BYTES.getDefaultValue().asInt()); | |||
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.
I agree
@ehsavoie updated the PR - since the transformation tests are being skipped I don't know what I can do there, so I only kept the one test for EAP 7.4. The only other relevant test I found is in the testsuite so I changed it to check if values are what we expect. Is there anything else? |
…d attributes, remove duplicate method
…plicate method
Issue:WFLY-20077