[WFLY-17180] JCA: add query-timeout validation parameter #543
Conversation
| {code} | ||
| <datasource (...)> | ||
| (...) | ||
| <validation query-timeout='...'> |
There was a problem hiding this comment.
Maybe we can create this fix will a bit less elaborate configuration?
There was a problem hiding this comment.
I agree. I can change the PR accordingly.
There was a problem hiding this comment.
Please primarily discuss configuration in terms of the CLI.
| </validation> | ||
| (...) | ||
| </datasource> | ||
| {code} |
There was a problem hiding this comment.
Please move this entire section below under "Proposed Solution" (After you rename it to "Implementation Plan".
|
|
||
| Minimally, an explicit query timeout should be supplied. | ||
|
|
||
| It might also be useful if a pool property or perhaps a system property could support a configurable timeout period (e.g. to account for potential latency in some systems). The Apache Tomcat pool supports something of this nature with the validationQueryTimeout property. |
There was a problem hiding this comment.
Configuration via system property is not acceptable in WildFly features when there is an appropriate alternative. Our configuration API is the formal management API, supported deployment descriptors, or formally supported annotations etc.
Downstream we sometimes use system properties when introduction new customer-requested functionality cannot be done using a management API update. Typically that would be for features that some might argue are bug fixes. But there is no reason to account for that situation in proposals for new functionality in WildFly. This overview is somewhat speculative, i.e. discusses things that might be done, which is ok, but please drop this system properties bit.
|
|
||
| * customers should be able to configure validation-timeout parameter | ||
|
|
||
| == Proposed solution |
There was a problem hiding this comment.
Please use "Implementation Plan"
|
|
||
| == Requirements | ||
|
|
||
| * customers should be able to configure validation-timeout parameter |
There was a problem hiding this comment.
Please add the "Hard Requirements", "Nice-to-Have Requirements" and "Non-Requirements" sections from the template. Then please be more explicit.
The "Overview" talks about things in terms of "Minimally" and "It might also be useful" but in this section you need to say what is actually going to be done, using the 3 requirement categories. Any question in the readers' mind based on the Overview discussion should be removed when reading the requirements section.
This has been around for a while, so do I think you should say "None" under "Nice-to-Have Requirements". Any ideas are either in-scope for this iteration, or are out of scope. Use "Non-Requirements" for things you thought about that you decided are not in scope.
In the "Implementation Plan" section adding a "Future Work" section is sometimes useful, as way of noting things that might get done in some future iteration. Besides being good info this helps emphasize that things that might seem reasonable are not part of this iteration.
|
|
||
| === Test Plan | ||
|
|
||
| * Connector subsystem unit tests will be extended to WildFly integration testsuite. The goal of the test would be to verify that this attribute is parsed and applied correctly. |
There was a problem hiding this comment.
Can you add this (assuming I'm correct and it's true)?
"The actual validation timeout handling is done in IronJacamar and has been thoroughly tested and available for many releases now. Therefore the test plan for this proposal does not cover such testing."
|
|
||
| === Community docs | ||
|
|
||
| * None |
There was a problem hiding this comment.
Please say something like:
"The new attribute will be documented in wildscribe".
That's the boilerplate info for "add a new attribute" proposals where no further documentation is necessary.
| @@ -0,0 +1,72 @@ | |||
| = WFLY-17180 Add query-timeout validation parameter | |||
There was a problem hiding this comment.
Please add appropriate Jekyll front matter so this ends up correctly categorized on the docs.wildfly.org/wildfly-proposals page.
| === Related Issues: | ||
|
|
||
| * {issue-base-url}/EAP7-2166[EAP7-2166] | ||
|
|
There was a problem hiding this comment.
Please add the new 'Stability' section as per, with the desired level selected.
https://github.com/wildfly/wildfly-proposals/blob/main/design-doc-template.adoc#stability-level
I suggest 'Community'.
If the selection is other than 'Default', please preface the L1 heading with "[the level"], e.g. "[Community]WFLY-17180 Add query-timeout validation parameter"
|
@bstansberry @tadamski I will fix this. It slipped out of my radar. |
|
@istudens That's understandable. With the new WildFly feature development process things like this that have been stuck for a long time can more forward. I'm not sure but I believe this one is mostly done except for some paperwork stuff, like this doc. |
https://issues.redhat.com/browse/WFLY-17180