[ 
https://issues.apache.org/jira/browse/QPID-8666?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17814467#comment-17814467
 ] 

ASF GitHub Bot commented on QPID-8666:
--------------------------------------

gemmellr commented on code in PR #239:
URL: https://github.com/apache/qpid-broker-j/pull/239#discussion_r1478660448


##########
broker-core/src/main/java/org/apache/qpid/server/store/UpgraderHelper.java:
##########
@@ -79,32 +80,45 @@ public static ConfiguredObjectRecord 
upgradeConnectionPool(final ConfiguredObjec
     {
         final Map<String, Object> attributes = record.getAttributes();
 
+        final Map<String, Object> updatedAttributes = new 
HashMap<>(record.getAttributes());
+        if (BONECP.equals(attributes.get(CP_TYPE)))
+        {
+            updatedAttributes.put(CP_TYPE, HIKARICP);
+        }
+
         final Object contextObject = attributes.get(CONTEXT);
 
         if (contextObject instanceof Map)
         {
             final Map <String, String> context = (Map<String, String>) 
contextObject;
             final Map<String, String> newContext = 
UpgraderHelper.renameContextVariables(context, RENAME_MAPPING);
 
+            final int partitionCount = newContext.get(PARTITION_COUNT_PARAM) 
!= null
+                    ? 
Integer.parseInt(String.valueOf(newContext.remove(PARTITION_COUNT_PARAM))) : 0;
+            final int maximumPoolSize = newContext.get(MAX_POOL_SIZE_PARAM) != 
null && partitionCount != 0
+                    ? 
Integer.parseInt(String.valueOf(newContext.get(MAX_POOL_SIZE_PARAM))) * 
partitionCount : 40;
+            final int minIdle = newContext.get(MIN_IDLE_PARAM) != null && 
partitionCount != 0
+                    ? 
Integer.parseInt(String.valueOf(newContext.get(MIN_IDLE_PARAM))) * 
partitionCount : 20;
+
             if (BONECP.equals(attributes.get(CP_TYPE)))
             {
-                final int partitionCount = 
newContext.get(PARTITION_COUNT_PARAM) != null
-                        ? 
Integer.parseInt(newContext.remove(PARTITION_COUNT_PARAM)) : 0;
-                final int maximumPoolSize = 
newContext.get(MAX_POOL_SIZE_PARAM) != null && partitionCount != 0
-                        ? 
Integer.parseInt(newContext.get(MAX_POOL_SIZE_PARAM)) * partitionCount : 40;
-                final int minIdle = newContext.get(MIN_IDLE_PARAM) != null && 
partitionCount != 0
-                        ? Integer.parseInt(newContext.get(MIN_IDLE_PARAM)) * 
partitionCount : 20;
                 newContext.put(MAX_POOL_SIZE_PARAM, 
String.valueOf(maximumPoolSize));
                 newContext.put(MIN_IDLE_PARAM, String.valueOf(minIdle));
             }
-            final Map<String, Object> updatedAttributes = new 
HashMap<>(record.getAttributes());
-            if (BONECP.equals(attributes.get(CP_TYPE)))
+            else if ("Broker".equals(record.getType()))

Review Comment:
   This comment is about the if above it (new lines 103-107), rather than this 
else if itself, i.e about:
   ```
               if (BONECP.equals(attributes.get(CP_TYPE)))
               {
                   newContext.put(MAX_POOL_SIZE_PARAM, 
String.valueOf(maximumPoolSize));
                   newContext.put(MIN_IDLE_PARAM, String.valueOf(minIdle));
               }
   ```
   Doesnt that mean it _always_ adds the new context variables, if the old type 
was BONECP? Even if the old config values werent actually there?
   
   Isnt it possible for the context values to be inherited or defaulted? If so, 
shouldnt they only be added to a given context if actually replacing prior old 
values that are present in it?
   
   (e.g like the following code for "Broker", only inserts the calculated value 
if the key was present...shouldnt it just always do that check rather than 
special-case it?)





> [Broker-J] Broker plugin jdbc-provider-bone replacement
> -------------------------------------------------------
>
>                 Key: QPID-8666
>                 URL: https://issues.apache.org/jira/browse/QPID-8666
>             Project: Qpid
>          Issue Type: Improvement
>          Components: Broker-J
>    Affects Versions: qpid-java-broker-9.1.0
>            Reporter: Daniil Kirilyuk
>            Priority: Major
>             Fix For: qpid-java-broker-9.2.0
>
>
> Broker plugin jdbc-provider-bone relies on JDBC connection pooling library 
> bonecp, which is no longer developed or updated. The [github project 
> page|https://github.com/wwadge/bonecp] states:
> "BoneCP is a Java JDBC connection pool implementation that is tuned for high 
> performance by minimizing lock contention to give greater throughput for your 
> applications. It beats older connection pools such as C3P0 and DBCP but 
> should now be considered deprecated in favour of HikariCP".
> Plugin jdbc-provider-bone should be replaced with the new one 
> jdbc-provider-hikaricp based on 
> [HikariCP|https://github.com/brettwooldridge/HikariCP] library. Both 
> libraries use Apache-2.0 license.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

Reply via email to