WFLY-10009 Introduce a maximum-timeout management model attribute #11440

tomjenkinson · 2018-07-18T19:22:08Z

https://issues.jboss.org/browse/WFLY-10009

Should likely replace #10998 if it passes.

Thanks for submitting your Pull Request!

Please make sure your PR meets the following requirements:

Pull Request title is properly formatted: [WFLY-XYZ] Subject or WFLY-XYZ Subject
Pull Request contains link to the JIRA issue(s)
Pull Request contains description of the issue(s)
Pull Request does not include fixes for issues other than the main ticket
Attached commits represent units of work and are properly formatted

For bigger changes, major and minor component upgrades make sure your PR also meets following requirements:

Pull Request requires a change to the documentation
Documentation have been updated accordingly
Tests were added to cover changes

For new features ensure as well:

Analysis was done
Test Plan has been done
Tests were verified in advance

If you are not an active contributor of the WildFly project you can request sponsorship by one of the members to help guide you through the process.

jamezp · 2018-07-18T20:06:05Z

...on/basic/src/test/java/org/jboss/as/test/integration/transaction/MaximumTimeoutTestCase.java

+@ServerSetup({MaximumTimeoutTestCase.TimeoutSetup.class})
+public class MaximumTimeoutTestCase {
+
+    private static final String MESSAGE_REGEX = ".*\\bWARN\\b.*WFLYTX0039: The transaction timeout has been set to %d while the value is 0";


I don't really like matching messages in a test as it's fragile. If it's necessary though we should only be matching the id. This would break with any internationalized languages.

+1, we should just match on the ID if anything

Having looked at this test more it seems meaningful to verify the two options in this integration test.

The main issue is once translated interfaces are introduced this would break for anyone running the test with a translated local, e.g. fr. A pattern validating the number is okay, but we shouldn't use the English text.

Is that even the case for tests? Do we run localized tests?

The issue will show up in EAP where we do introduce localization. While CI should continue to work anyone building locally with a locale that's been translated will see this test fail. For example anyone in France with a French locale will see this test fail.

OK, I will push a change

jamezp · 2018-07-18T20:17:27Z

transactions/src/main/java/org/jboss/as/txn/logging/TransactionLogger.java

@@ -250,4 +250,7 @@
    @Message(id = 38, value = "Unexpected error on suspending transaction for work %s")
    RuntimeException cannotSuspendInflowTransactionUnexpectedError(Work txn, @Cause Exception e);

+    @LogMessage(level = WARN)
+    @Message(id = 39, value = "The transaction timeout has been set to %s while the value is 0")


I'm not really user I understand this message. Specifically "while the value is 0".

Nor do I, @zhfeng can you take a look please?

@jamezp @tomjenkinson This message is warning the user that the transaction timeout can not be set to "0" and will replace with the maximum-timeout attribute.

@zhfeng Those attributes should probably be listed then. Just so it's clear for the user.

jamezp · 2018-07-18T20:27:42Z

transactions/src/main/resources/org/jboss/as/txn/subsystem/LocalDescriptions.properties

@@ -34,6 +34,7 @@ transactions.enable-statistics=Whether transaction statistics should be gathered
 transactions.enable-statistics.deprecated=Use statistics-enabled.
 transactions.enable-tsm-status=Whether the transaction status manager (TSM) service, needed for out of process recovery, should be provided or not.
 transactions.default-timeout=The default timeout for a transaction managed by the transaction manager.
+transactions.maximum-timeout=The maximum timeout for a transaction managed by the transaction manager.


This may need to be a bit more verbose. It looks like this value is only used if the default-timeout is set to 0.

OK, I will change this description to make it clear.

jamezp · 2018-07-18T20:28:50Z

transactions/src/test/java/org/jboss/as/txn/subsystem/TransactionSubsystemTestCase.java

@@ -298,6 +315,7 @@ private ModelNode createReadAttributeOperation(String name) {
        modelNode.get(TransactionSubsystemRootResourceDefinition.JOURNAL_STORE_ENABLE_ASYNC_IO.getName()).set(true);
        modelNode.get(TransactionSubsystemRootResourceDefinition.USE_HORNETQ_STORE.getName()).set(true);
        modelNode.get(TransactionSubsystemRootResourceDefinition.USE_JOURNAL_STORE.getName()).set(true);
+//        modelNode.get(TransactionSubsystemRootResourceDefinition.MAXIMUM_TIMEOUT.getName()).set(31536000);


This should be un-commented or removed.

I will do that now

kabir · 2018-07-19T08:15:55Z

transactions/src/test/java/org/jboss/as/txn/subsystem/TransactionSubsystemTestCase.java

-                        FailedOperationTransformationConfig.REJECTED_RESOURCE));
+    public void testRejectTransformersEAP640() throws Exception {
+        testRejectTransformers(ModelTestControllerVersion.EAP_6_4_0, MODEL_VERSION_EAP64, new FailedOperationTransformationConfig().addFailedAttribute(
+                PathAddress.pathAddress(TransactionExtension.SUBSYSTEM_PATH), new FailedOperationTransformationConfig.NewAttributesConfig("maximum-timeout")));


Although not caused by this PR, we might as well fix the following. 7.0.0+ introduces the following attributes not found in 6.4.0 (it is in our mail thread): [~~maximum-timeout,~~ use-journal-store, statistics-enabled, journal-store-enable-async-io]. I crossed out maximum-timeout as that is being handled. But the test xml should be updated to include these and transformers added where missing

Perhaps we could do that in a follow up issue?

Yeah, it is more a bug than anything else. File a high-priority Jira and we're good ;)

Thanks Kabir - I filed: https://issues.jboss.org/browse/WFLY-10715

tomjenkinson · 2018-07-19T09:13:01Z

@zhfeng please can you take a look at @jamezp comments above? You can push commits to https://github.com/jbosstm/jboss-as/tree/WFLY-10009 to address them.

@jamezp thanks for the review so far!

zhfeng · 2018-07-23T13:32:37Z

Thanks @tomjenkinson for working on this issue.

kabir · 2018-07-23T14:16:58Z

@tomjenkinson @zhfeng Compilation error

tomjenkinson · 2018-07-24T11:28:30Z

Thanks all for the review, I hope the latest push resolves the outstanding issues.

tomjenkinson · 2018-07-25T13:39:53Z

transactions/src/main/java/org/jboss/as/txn/subsystem/TransactionExtension.java

@@ -124,14 +124,10 @@ public void initialize(ExtensionContext context) {
     */
    @Override
    public void initializeParsers(ExtensionParsingContext context) {
-        context.setSubsystemXmlMapping(SUBSYSTEM_NAME, Namespace.TRANSACTIONS_1_0.getUriString(), TransactionSubsystem10Parser::new);


@kabir I am not sure why we removed these but I think the removal causes the manualmode test to fail, I will add them back and see if it passes again

tomjenkinson · 2018-07-26T12:07:43Z

I am seeing that HotRod failing test is not new, as such could this be merged yet please?

/cc @kabir @jamezp

tomjenkinson · 2018-07-26T15:36:01Z

@jamezp thanks for the approval

kabir

@tomjenkinson @zhfeng I have a few final comments. Also, please squash the commits - 15 is a bit much for a simple change :)

kabir · 2018-07-30T08:36:42Z

testsuite/integration/basic/pom.xml

@@ -185,6 +185,7 @@
                                        <include>org/jboss/as/test/integration/ee/injection/resource/basic/*TestCase.java</include>
                                        <include>org/jboss/as/test/integration/ee/naming/defaultbindings/**/*TestCase.java</include>
                                        <include>org/jboss/as/test/integration/transaction/inflow/TransactionInflowTestCase.java</include>
+                                        <include>org/jboss/as/test/integration/transaction/MaximumTimeoutTestCase.java</include>


@tomjenkinson @zhfeng this also needs adding to the s of the basic-integration-default-web.surefire execution

done (I think) it was to add to exclude I think (the word you used before "s" is not showing in github UI correctly, maybe it is parsing some XML?)

kabir · 2018-07-30T08:49:17Z

transactions/src/main/java/org/jboss/as/txn/subsystem/TransactionTransformers.java

+        ResourceTransformationDescriptionBuilder builderEap71 = chainedBuilder.createBuilder(CURRENT_MODEL_VERSION, MODEL_VERSION_EAP71);
+        builderEap71.getAttributeBuilder()
+                .setDiscard(new DiscardAttributeChecker.DiscardAttributeValueChecker(MAXIMUM_TIMEOUT.getDefaultValue()), MAXIMUM_TIMEOUT)
+                .addRejectCheck(new RejectAttributeChecker.SimpleAcceptAttributeChecker(MAXIMUM_TIMEOUT.getDefaultValue()), MAXIMUM_TIMEOUT)


AFAICT this should be RejectAttributeChecker.DEFINED. The discard on the line above deals with the 'allowed' value. I've tried and the tests pass.

tomjenkinson · 2018-07-30T11:35:20Z

@kabir hopefully addresses your final comments

…nsaction subsystem

…ribute and only reject the non-default value.

jamezp · 2018-07-30T16:51:18Z

Just an FYI. The TransactionSubsystemTestCase was failing due to adding the maximum-timeout attribute to the 1.5.0 (EAP 6.4) model. I removed that and the commented out changes for the ModelFixer's. I rebased and forced push to the jbosstm/WFLY-10009 branch. Once CI passes we'll try to get this merged.

WFLY-10009 maximum timeout attribute

jamezp requested changes Jul 18, 2018

View reviewed changes

jamezp added the Feature PR provides a new feature label Jul 18, 2018

jamezp mentioned this pull request Jul 18, 2018

WFLY-10009 Introduce a maximum-timeout management model attribute #10998

Closed

11 tasks

kabir reviewed Jul 19, 2018

View reviewed changes

tomjenkinson force-pushed the WFLY-10009 branch from b268fdc to 398a8b2 Compare July 24, 2018 11:26

tomjenkinson commented Jul 25, 2018

View reviewed changes

tomjenkinson force-pushed the WFLY-10009 branch 2 times, most recently from 3445967 to 2d79d08 Compare July 25, 2018 16:28

jamezp approved these changes Jul 26, 2018

View reviewed changes

jamezp added ready-for-merge Only for use by those with merge permissions! and removed Feature PR provides a new feature labels Jul 26, 2018

jamezp added ready-for-merge Only for use by those with merge permissions! and removed ready-for-merge Only for use by those with merge permissions! labels Jul 26, 2018

wildfly deleted a comment from jamezp Jul 30, 2018

wildfly deleted a comment from tomjenkinson Jul 30, 2018

kabir reviewed Jul 30, 2018

View reviewed changes

kabir added fixme and removed ready-for-merge Only for use by those with merge permissions! labels Jul 30, 2018

tomjenkinson force-pushed the WFLY-10009 branch from 2d79d08 to c5d3191 Compare July 30, 2018 11:34

tomjenkinson force-pushed the WFLY-10009 branch from c5d3191 to 132515b Compare July 30, 2018 14:11

zhfeng and others added 2 commits July 30, 2018 09:47

WFLY-10009 Update to introduce a maximum timeout attribute in the tra…

3614d35

…nsaction subsystem

WFLY-10009 maximum timeout attribute

cf36234

istraka and others added 3 commits July 30, 2018 09:47

[WFLY-10009] test maximum timeout attribute

6c4866d

[WFLY-10009] Discard the default value of the new maximum-timeout att…

2ae5625

…ribute and only reject the non-default value.

WFLY-10009 maximum timeout attribute

fa6cab6

jamezp force-pushed the WFLY-10009 branch from 132515b to fa6cab6 Compare July 30, 2018 16:48

jamezp added ready-for-merge Only for use by those with merge permissions! and removed fixme labels Jul 30, 2018

jamezp added a commit that referenced this pull request Jul 30, 2018

Merged pull request #11440

406afa4

WFLY-10009 maximum timeout attribute

jamezp merged commit 3e0ee5b into wildfly:master Jul 30, 2018

zhfeng mentioned this pull request Jul 31, 2018

WFLY-10009 Introduce a maximum-timeout management model attribute wildfly/wildfly-proposals#51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WFLY-10009 Introduce a maximum-timeout management model attribute #11440

WFLY-10009 Introduce a maximum-timeout management model attribute #11440

tomjenkinson commented Jul 18, 2018

jamezp Jul 18, 2018

tomjenkinson Jul 19, 2018

tomjenkinson Jul 24, 2018

jamezp Jul 24, 2018

tomjenkinson Jul 25, 2018

jamezp Jul 25, 2018

tomjenkinson Jul 25, 2018

jamezp Jul 18, 2018

tomjenkinson Jul 19, 2018

zhfeng Jul 23, 2018

jamezp Jul 23, 2018

jamezp Jul 18, 2018

tomjenkinson Jul 19, 2018

zhfeng Jul 23, 2018

jamezp Jul 18, 2018

tomjenkinson Jul 19, 2018

kabir Jul 19, 2018 •

edited

tomjenkinson Jul 19, 2018

kabir Jul 19, 2018

tomjenkinson Jul 19, 2018

tomjenkinson commented Jul 19, 2018

zhfeng commented Jul 23, 2018

kabir commented Jul 23, 2018

tomjenkinson commented Jul 24, 2018

tomjenkinson Jul 25, 2018

tomjenkinson commented Jul 26, 2018

tomjenkinson commented Jul 26, 2018

kabir left a comment

kabir Jul 30, 2018

tomjenkinson Jul 30, 2018

kabir Jul 30, 2018

tomjenkinson commented Jul 30, 2018

jamezp commented Jul 30, 2018

WFLY-10009 Introduce a maximum-timeout management model attribute #11440

WFLY-10009 Introduce a maximum-timeout management model attribute #11440

Conversation

tomjenkinson commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kabir Jul 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomjenkinson commented Jul 19, 2018

zhfeng commented Jul 23, 2018

kabir commented Jul 23, 2018

tomjenkinson commented Jul 24, 2018

Choose a reason for hiding this comment

tomjenkinson commented Jul 26, 2018

tomjenkinson commented Jul 26, 2018

kabir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomjenkinson commented Jul 30, 2018

jamezp commented Jul 30, 2018

kabir Jul 19, 2018 •

edited