Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WFLY-17180] JCA: add query-timeout validation parameter #543

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[WFLY-17180] JCA: add query-timeout validation parameter #543

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions jca/WFLY-17180_Add_queryTimeout_validation_parameter.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
= WFLY-17180 Add query-timeout validation parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add appropriate Jekyll front matter so this ends up correctly categorized on the docs.wildfly.org/wildfly-proposals page.

:author: Tomasz Adamski
:email: [email protected]
:toc: left
:icons: font
:keywords: connector, validation
:idprefix:
:idseparator: -
:issue-base-url: https://issues.redhat.com/browse/

== Overview

Validation queries run without any explicit query timeout. If the validation query blocks (as it will with most drivers when a connection is invalid/stale), this can block the entire pool in certain cases (e.g. background-validation locks the pool during validation). Even with validate-on-match, the failure to enforce a short timeout can significantly impact time to get a connection if one or more failed connections are found in the pool.

Setting a driver level query timeout is not a suitable workaround because this impacts normal queries that may be expected to run for significant periods. The timeout for normal queries needs to be different from what is tolerated for validation queries (which should respond nearly instantaneously or else the connection is likely broken).

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


== Configuration

'query-timeout' attribute would be added to validation tag of datasources subsystem:

{code}
<datasource (...)>
(...)
<validation query-timeout='...'>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can create this fix will a bit less elaborate configuration?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I can change the PR accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please primarily discuss configuration in terms of the CLI.

(...)
</validation>
(...)
</datasource>
{code}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this entire section below under "Proposed Solution" (After you rename it to "Implementation Plan".



== Issue Metadata

=== Issue:

* {issue-base-url}/WFLY-17180[WFLY-17180]

=== Related Issues:

* {issue-base-url}/EAP7-2166[EAP7-2166]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

=== Dev Contacts:

* mailto:{email}[{author}]

=== Testing by:

* Engineering

=== Affected Projects or Components:

* IronJacamar, WildFly Connector

== Requirements

* customers should be able to configure validation-timeout parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


== Proposed solution
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use "Implementation Plan"

* modify WildFly Connector subystem datasource configuration to include 'query-timeout' attribute in 'validation' tag

=== 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.