[CLOUD-2455] - [BxMS] ERROR IJ000315: Pool amq-ConnectionFactory has … #226

spolti · 2018-04-10T17:52:21Z

…1 active handles

Signed-off-by: Filippe Spolti fspolti@redhat.com

Thanks for submitting your Pull Request!

Please make sure your PR meets following requirements:

Pull Request title is properly formatted: [CLOUD-XYA] Subject
Pull Request contains link to the JIRA issue
Pull Request contains description of the issue
Pull request does not include fixes for other issues than the main ticket
Attached commits represent unit of work and are properly formatted
You have read and agreed to the Developer Certificate of Origin (DCO) (see CONTRIBUTING.md)
Every commit contains Signed-off-by: Your Name <yourname@redhat.com> - use git commit -s

errantepiphany

Please see comments about potential changes.

errantepiphany · 2018-04-10T18:39:31Z

os-eap-launch/added/launch/resource-adapters-common.sh

-    resource_adapter="${resource_adapter}<connection-definitions><connection-definition class-name=\"${ra_class}\" jndi-name=\"${ra_jndi}\" enabled=\"true\" use-java-context=\"true\">"
+    resource_adapter="${resource_adapter}<connection-definitions><connection-definition"
+    # monitor applications, look for unclosed resources.
+    tracking=$(find_env "${ra_prefix}_TRACKING" "true")


I think the "tracking" variable can be declared as local, and moved into the "if [[ "$JBOSS_EAP_VERSION" == "7."* ]]" statement below it.

Shouldn't the default be "false", per the workaround described in CLOUD-2455?

agreed, thanks.

maschmid · 2018-04-11T13:54:20Z

os-eap-launch/added/launch/resource-adapters-common.sh

+
+    if [[ "$JBOSS_EAP_VERSION" == "7."* ]]; then
+      # monitor applications, look for unclosed resources.
+      local tracking=$(find_env "${ra_prefix}_TRACKING" "false")


we should keep the default and set it only if the property is set. (I believe in general it defaults to true, but perhaps not always)

After discussion, I agree with @maschmid . Instead of checking the jboss eap version, just see if the tracking variable is set. If it is, set it, otherwise, do nothing. Additionally, instead of having to change all our KIE templates to set this variable to false, we can just set them in the image.yaml (not even needing to declare them in the templates).

The problem of doing this is that it will also be applied on eap6, is this setting also necessary on eap6?

The idea is that we wouldn't set the variable unless the env var is defined (so, don't use a default value when looking up the env var). It then wouldn't be applied to eap6, because no one would be setting the env var.

maschmid · 2018-04-11T14:25:09Z

Make sure you also don't need to change

https://github.com/jboss-openshift/cct_module/blob/master/os-eap7-launch/added/launch/messaging.sh#L174

(or do that same / similar change there if you need)

@maschmid

Dismissing my review in favor of @maschmid's.

maschmid · 2018-04-11T19:35:45Z

os-eap-launch/added/launch/resource-adapters-common.sh

-    resource_adapter="${resource_adapter}<connection-definitions><connection-definition class-name=\"${ra_class}\" jndi-name=\"${ra_jndi}\" enabled=\"true\" use-java-context=\"true\">"
+    resource_adapter="${resource_adapter}<connection-definitions><connection-definition"
+
+    if [ -n "${RA_TRACKING}" ]; then


in this case this is resource-adapter specific, so it should use the ra_prefix_TRACKING

maschmid · 2018-04-11T19:41:23Z

os-eap7-launch/added/launch/messaging.sh

@@ -313,14 +315,18 @@ function inject_brokers() {
      # topics environment variable name format: [NAME]_[BROKER_TYPE]_TOPICS
      topics=$(find_env "${prefix}_TOPICS")

+      if [ -n "${RA_TRACKING}" ]; then


should be specific to the resource adapter, as in general there can be more (should use the ${prefix}_TRACKING

…1 active handles Signed-off-by: Filippe Spolti <fspolti@redhat.com>

maschmid · 2018-04-12T08:44:36Z

os-eap7-launch/added/launch/messaging.sh

@@ -172,6 +173,7 @@ function generate_resource_adapter() {
                    <config-property name=\"ServerUrl\">tcp://$6:$7?jms.rmIdFromConnectionId=true</config-property>
                    <connection-definitions>
                        <connection-definition
+                              "${13}"


this is already inside quotes , did you mean just ${13} ?

Its ok use quotes there since it is not escaped.

maschmid · 2018-04-12T08:45:26Z

os-eap7-launch/added/launch/messaging.sh

+      tracking=$(find_env "${prefix}_TRACKING")
+      if [ -n "${tracking}" ]; then
+         ra_tracking="tracking=\"${tracking}\""
+      fi


ra_tracking is potentially undefined here, it would be better to explicitly set it to an empty string, just to be safe from a global "ra_tracking" set somewhere else...

It is a hard scenario but can happen since this var is not local, I'll updated it, does it block the QE to test using the images that is already there?

no I don't think we need to retest on trivial changes, as long as the container behave tests pass.

rcernich · 2018-04-12T14:03:50Z

os-eap-launch/added/launch/resource-adapters-common.sh

-    resource_adapter="${resource_adapter}<connection-definitions><connection-definition class-name=\"${ra_class}\" jndi-name=\"${ra_jndi}\" enabled=\"true\" use-java-context=\"true\">"
+    resource_adapter="${resource_adapter}<connection-definitions><connection-definition"
+
+    tracking=$(find_env "${ra_prefix}_TRACKING")


This should be local.

never mind. it looks like this is initialized every time.

errantepiphany self-requested a review April 10, 2018 18:36

errantepiphany previously requested changes Apr 10, 2018

View reviewed changes

spolti force-pushed the CLOUD-2455 branch from 8b27af8 to 7668604 Compare April 10, 2018 20:16

maschmid reviewed Apr 11, 2018

View reviewed changes

spolti force-pushed the CLOUD-2455 branch from 7668604 to 8ef7350 Compare April 11, 2018 17:31

luck3y approved these changes Apr 11, 2018

View reviewed changes

spolti force-pushed the CLOUD-2455 branch from 8ef7350 to 4f3588b Compare April 11, 2018 19:07

maschmid suggested changes Apr 11, 2018

View reviewed changes

[CLOUD-2455] - [BxMS] ERROR IJ000315: Pool amq-ConnectionFactory has …

151a187

…1 active handles Signed-off-by: Filippe Spolti <fspolti@redhat.com>

spolti force-pushed the CLOUD-2455 branch from 4f3588b to 151a187 Compare April 11, 2018 21:03

maschmid reviewed Apr 12, 2018

View reviewed changes

rcernich reviewed Apr 12, 2018

View reviewed changes

rcernich approved these changes Apr 12, 2018

View reviewed changes

rcernich merged commit 1f1bf53 into jboss-openshift:master Apr 12, 2018

spolti deleted the CLOUD-2455 branch April 13, 2021 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLOUD-2455] - [BxMS] ERROR IJ000315: Pool amq-ConnectionFactory has … #226

[CLOUD-2455] - [BxMS] ERROR IJ000315: Pool amq-ConnectionFactory has … #226

spolti commented Apr 10, 2018

errantepiphany left a comment

errantepiphany Apr 10, 2018 •

edited

spolti Apr 10, 2018

maschmid Apr 11, 2018 •

edited

errantepiphany Apr 11, 2018

spolti Apr 11, 2018

errantepiphany Apr 11, 2018 •

edited

maschmid commented Apr 11, 2018

maschmid Apr 11, 2018

maschmid Apr 11, 2018

maschmid Apr 12, 2018

spolti Apr 12, 2018

maschmid Apr 12, 2018 •

edited

spolti Apr 12, 2018

maschmid Apr 12, 2018 •

edited

rcernich Apr 12, 2018

rcernich Apr 12, 2018

[CLOUD-2455] - [BxMS] ERROR IJ000315: Pool amq-ConnectionFactory has … #226

[CLOUD-2455] - [BxMS] ERROR IJ000315: Pool amq-ConnectionFactory has … #226

Conversation

spolti commented Apr 10, 2018

errantepiphany left a comment

Choose a reason for hiding this comment

errantepiphany Apr 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maschmid Apr 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

errantepiphany Apr 11, 2018 • edited

Choose a reason for hiding this comment

maschmid commented Apr 11, 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

maschmid Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maschmid Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

errantepiphany Apr 10, 2018 •

edited

maschmid Apr 11, 2018 •

edited

errantepiphany Apr 11, 2018 •

edited

maschmid Apr 12, 2018 •

edited

maschmid Apr 12, 2018 •

edited