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?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
