[PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
dakirily opened a new pull request, #240: URL: https://github.com/apache/qpid-broker-j/pull/240 This PR addresses JIRA [QPID-8666](https://issues.apache.org/jira/browse/QPID-8666), adding a UI fix for the issue when creating a JSON Virtual Host Node with BDB Virtual host resulting in "dijit.registry.byId(...) is undefined" error. -- 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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
gemmellr commented on code in PR #239:
URL: https://github.com/apache/qpid-broker-j/pull/239#discussion_r1479641958
##
broker-core/src/main/java/org/apache/qpid/server/store/UpgraderHelper.java:
##
@@ -79,32 +80,45 @@ public static ConfiguredObjectRecord
upgradeConnectionPool(final ConfiguredObjec
{
final Map attributes = record.getAttributes();
+final Map 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 context = (Map)
contextObject;
final Map 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 updatedAttributes = new
HashMap<>(record.getAttributes());
-if (BONECP.equals(attributes.get(CP_TYPE)))
+else if ("Broker".equals(record.getType()))
Review Comment:
looks good, merged
--
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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
gemmellr merged PR #239: URL: https://github.com/apache/qpid-broker-j/pull/239 -- 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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
dakirily commented on code in PR #239:
URL: https://github.com/apache/qpid-broker-j/pull/239#discussion_r1479388677
##
broker-core/src/main/java/org/apache/qpid/server/store/UpgraderHelper.java:
##
@@ -79,32 +80,45 @@ public static ConfiguredObjectRecord
upgradeConnectionPool(final ConfiguredObjec
{
final Map attributes = record.getAttributes();
+final Map 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 context = (Map)
contextObject;
final Map 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 updatedAttributes = new
HashMap<>(record.getAttributes());
-if (BONECP.equals(attributes.get(CP_TYPE)))
+else if ("Broker".equals(record.getType()))
Review Comment:
Variable name were renamed appropriately.
The upgrader code was changed to make logic for BONECP old type and "Broker"
entity the same.
--
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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
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 attributes = record.getAttributes();
+final Map 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 context = (Map)
contextObject;
final Map 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 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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
dakirily commented on code in PR #239:
URL: https://github.com/apache/qpid-broker-j/pull/239#discussion_r1478477230
##
broker-core/src/test/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecovererTest.java:
##
@@ -898,10 +897,75 @@ public void
testContextVariableUpgradeFromBoneCPToHikariCPProvider(final String
context.put("qpid.jdbcstore.bonecp.partitionCount", partitionCount);
context.put("qpid.jdbcstore.bonecp.maxConnectionsPerPartition",
maxConnectionsPerPartition);
context.put("qpid.jdbcstore.bonecp.minConnectionsPerPartition",
minConnectionsPerPartition);
+context.put("qpid.jdbcstore.property1", "1");
+context.put("qpid.jdbcstore.property2", "two");
+context.put("qpid.jdbcstore.property3", "_3_");
+
final Map attributes = Map.of("name", getTestName(),
"type", "JDBC",
"connectionPoolType", "BONECP",
"context", context);
+
+_brokerRecord.getAttributes().put("context", context);
+
+final ConfiguredObjectRecord systemConfigRecord =
mock(ConfiguredObjectRecord.class);;
+when(systemConfigRecord.getId()).thenReturn(randomUUID());
+when(systemConfigRecord.getType()).thenReturn("SystemConfig");
+
when(systemConfigRecord.getAttributes()).thenReturn(Map.copyOf(attributes));
Review Comment:
The test was fixed to use a copy of a context map for each object.
##
broker-core/src/test/java/org/apache/qpid/server/store/VirtualHostStoreUpgraderAndRecovererTest.java:
##
@@ -323,29 +323,36 @@ public void
testContextVariableUpgradeFromBoneCPToHikariCPProvider(final String
context.put("qpid.jdbcstore.bonecp.partitionCount", partitionCount);
context.put("qpid.jdbcstore.bonecp.maxConnectionsPerPartition",
maxConnectionsPerPartition);
context.put("qpid.jdbcstore.bonecp.minConnectionsPerPartition",
minConnectionsPerPartition);
+
final Map attributes = Map.of("name", getTestName(),
"modelVersion", "9.0",
"type", "JDBC",
"connectionPoolType", "BONECP",
"context", context);
+
final ConfiguredObjectRecord virtualHostRecord =
mock(ConfiguredObjectRecord.class);;
when(virtualHostRecord.getId()).thenReturn(randomUUID());
when(virtualHostRecord.getType()).thenReturn("VirtualHost");
when(virtualHostRecord.getAttributes()).thenReturn(attributes);
-final List records = List.of(rootRecord,
virtualHostRecord);
+final ConfiguredObjectRecord virtualHostLoggerRecord =
mock(ConfiguredObjectRecord.class);;
+when(virtualHostLoggerRecord.getId()).thenReturn(randomUUID());
+
when(virtualHostLoggerRecord.getType()).thenReturn("VirtualHostLogger");
+when(virtualHostLoggerRecord.getAttributes()).thenReturn(attributes);
Review Comment:
The test was fixed in the same way as in BrokerStoreUpgraderAndRecovererTest
to keep to uniformity.
--
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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
gemmellr commented on code in PR #239:
URL: https://github.com/apache/qpid-broker-j/pull/239#discussion_r1478374918
##
broker-core/src/test/java/org/apache/qpid/server/store/VirtualHostStoreUpgraderAndRecovererTest.java:
##
@@ -323,29 +323,36 @@ public void
testContextVariableUpgradeFromBoneCPToHikariCPProvider(final String
context.put("qpid.jdbcstore.bonecp.partitionCount", partitionCount);
context.put("qpid.jdbcstore.bonecp.maxConnectionsPerPartition",
maxConnectionsPerPartition);
context.put("qpid.jdbcstore.bonecp.minConnectionsPerPartition",
minConnectionsPerPartition);
+
final Map attributes = Map.of("name", getTestName(),
"modelVersion", "9.0",
"type", "JDBC",
"connectionPoolType", "BONECP",
"context", context);
+
final ConfiguredObjectRecord virtualHostRecord =
mock(ConfiguredObjectRecord.class);;
when(virtualHostRecord.getId()).thenReturn(randomUUID());
when(virtualHostRecord.getType()).thenReturn("VirtualHost");
when(virtualHostRecord.getAttributes()).thenReturn(attributes);
-final List records = List.of(rootRecord,
virtualHostRecord);
+final ConfiguredObjectRecord virtualHostLoggerRecord =
mock(ConfiguredObjectRecord.class);;
+when(virtualHostLoggerRecord.getId()).thenReturn(randomUUID());
+
when(virtualHostLoggerRecord.getType()).thenReturn("VirtualHostLogger");
+when(virtualHostLoggerRecord.getAttributes()).thenReturn(attributes);
Review Comment:
This test isnt taking steps to copy the attributes map (or again the context
map), as was just introduced in the other test, seems strange for them to
differ in that sense when doing essentially the same.
##
broker-core/src/test/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecovererTest.java:
##
@@ -898,10 +897,75 @@ public void
testContextVariableUpgradeFromBoneCPToHikariCPProvider(final String
context.put("qpid.jdbcstore.bonecp.partitionCount", partitionCount);
context.put("qpid.jdbcstore.bonecp.maxConnectionsPerPartition",
maxConnectionsPerPartition);
context.put("qpid.jdbcstore.bonecp.minConnectionsPerPartition",
minConnectionsPerPartition);
+context.put("qpid.jdbcstore.property1", "1");
+context.put("qpid.jdbcstore.property2", "two");
+context.put("qpid.jdbcstore.property3", "_3_");
+
final Map attributes = Map.of("name", getTestName(),
"type", "JDBC",
"connectionPoolType", "BONECP",
"context", context);
+
+_brokerRecord.getAttributes().put("context", context);
+
+final ConfiguredObjectRecord systemConfigRecord =
mock(ConfiguredObjectRecord.class);;
+when(systemConfigRecord.getId()).thenReturn(randomUUID());
+when(systemConfigRecord.getType()).thenReturn("SystemConfig");
+
when(systemConfigRecord.getAttributes()).thenReturn(Map.copyOf(attributes));
Review Comment:
The test is taking care to copy the attributes map used for each record, but
the copies still all then contain the same context original map which isnt
copied. Should it be, to check each record is handled as expected, and given
the test later asserts on each record's context as if they are different maps?
--
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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
dakirily commented on code in PR #238:
URL: https://github.com/apache/qpid-broker-j/pull/238#discussion_r1478308423
##
broker-core/src/test/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecovererTest.java:
##
@@ -121,9 +124,10 @@ public void
testUpgradeVirtualHostWithJDBCStoreAndHikariCPPool()
"qpid.jdbcstore.varBinaryType", "myvarbinary",
"qpid.jdbcstore.blobType", "myblob",
"qpid.jdbcstore.useBytesForBlob", true,
-"qpid.jdbcstore.hikaricp.maximumPoolSize", 7,
-"qpid.jdbcstore.hikaricp.minimumIdle", 6);
-final Map expectedAttributes =
Map.of("connectionPoolType", "HIKARICP",
+"qpid.jdbcstore.bonecp.maxConnectionsPerPartition", 7,
+"qpid.jdbcstore.bonecp.minConnectionsPerPartition", 6,
+"qpid.jdbcstore.bonecp.partitionCount", 2);
+final Map expectedAttributes =
Map.of("connectionPoolType", "BONECP",
Review Comment:
Hi Robbie,
[PR-239](https://github.com/apache/qpid-broker-j/pull/239) was created to
fix those issues.
--
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
[PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
dakirily opened a new pull request, #239: URL: https://github.com/apache/qpid-broker-j/pull/239 This PR is a follow-up to the [PR-235](https://github.com/apache/qpid-broker-j/pull/235) and [PR-238](https://github.com/apache/qpid-broker-j/pull/238) and addresses JIRA [QPID-8666](https://issues.apache.org/jira/browse/QPID-8666), adding upgrading the broker model version from 9.0 to 9.1 -- 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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
gemmellr commented on code in PR #238:
URL: https://github.com/apache/qpid-broker-j/pull/238#discussion_r1476106471
##
broker-core/src/test/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecovererTest.java:
##
@@ -121,9 +124,10 @@ public void
testUpgradeVirtualHostWithJDBCStoreAndHikariCPPool()
"qpid.jdbcstore.varBinaryType", "myvarbinary",
"qpid.jdbcstore.blobType", "myblob",
"qpid.jdbcstore.useBytesForBlob", true,
-"qpid.jdbcstore.hikaricp.maximumPoolSize", 7,
-"qpid.jdbcstore.hikaricp.minimumIdle", 6);
-final Map expectedAttributes =
Map.of("connectionPoolType", "HIKARICP",
+"qpid.jdbcstore.bonecp.maxConnectionsPerPartition", 7,
+"qpid.jdbcstore.bonecp.minConnectionsPerPartition", 6,
+"qpid.jdbcstore.bonecp.partitionCount", 2);
+final Map expectedAttributes =
Map.of("connectionPoolType", "BONECP",
Review Comment:
Doesnt the fact these bits of the output didnt change (now restored to their
previous/original values), even though the updater is meant to be handling
transition from bonecp to hikaricp pool, suggest that not everything that
should be upgraded is being so?
EDIT: so this test is checking for "VirtualHostNode" context, whereas the
updater is only covering "VirtualHost" context entries, which seems likely to
explain things. Given both can apparently separately be using JDBC and thus
pooling (even if I dont see a point for the node) it feels like both should be
covered.
EDIT2: Actually, can the broker have this context config as well? to be
defined in one place and inherited by the rest if they use it and not override?
--
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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
vavrtom merged PR #238: URL: https://github.com/apache/qpid-broker-j/pull/238 -- 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
[PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
dakirily opened a new pull request, #238: URL: https://github.com/apache/qpid-broker-j/pull/238 This PR is a follow-up to the [PR 235](https://github.com/apache/qpid-broker-j/pull/235) and addresses JIRA [QPID-8666](https://issues.apache.org/jira/browse/QPID-8666), adding upgrading the broker model version from 9.0 to 9.1 -- 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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
gemmellr commented on code in PR #235:
URL: https://github.com/apache/qpid-broker-j/pull/235#discussion_r1474621213
##
broker-core/src/main/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecoverer.java:
##
@@ -805,11 +805,10 @@ private static class VirtualHostEntryUpgrader
addAttributeTransformer("jdbcBytesForBlob",
addContextVar("qpid.jdbcstore.useBytesForBlob")).
addAttributeTransformer("jdbcBlobType",
addContextVar("qpid.jdbcstore.blobType")).
addAttributeTransformer("jdbcVarbinaryType",
addContextVar("qpid.jdbcstore.varBinaryType")).
-addAttributeTransformer("partitionCount",
addContextVar("qpid.jdbcstore.bonecp.partitionCount")).
-addAttributeTransformer("maxConnectionsPerPartition",
-
addContextVar("qpid.jdbcstore.bonecp.maxConnectionsPerPartition")).
-addAttributeTransformer("minConnectionsPerPartition",
-
addContextVar("qpid.jdbcstore.bonecp.minConnectionsPerPartition")),
+addAttributeTransformer("maximumPoolSize",
+
addContextVar("qpid.jdbcstore.hikaricp.maximumPoolSize")).
+addAttributeTransformer("minimumIdle",
+
addContextVar("qpid.jdbcstore.hikaricp.minimumIdle")),
Review Comment:
I'm not 100% sure, its a long time since I really contributed to the broker
and things changed since then. Originally when we added that, I believe it was
just its own independent config version. From
broker-core/src/main/java/org/apache/qpid/server/model/BrokerModel.java it
looks like around the 6.0.0 release (when the release version was bumped as
everything became an independent component, rather than the big 'qpid release'
with everything) that the model version was also jumped to bring it into 'major
6, minor 0' alignment, and since then it seems like it has consistently had
changes that mean the major matched the broker release major version. It looks
like maybe the minors too. Perhaps digging more at the changes that have been
made historically will make it clearer for you what would be best, but using
either 9.1 (next minor) or 9.2 (matching minor) seems fair if the release
version is 9.2.0.
--
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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
gemmellr commented on code in PR #235:
URL: https://github.com/apache/qpid-broker-j/pull/235#discussion_r1474621213
##
broker-core/src/main/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecoverer.java:
##
@@ -805,11 +805,10 @@ private static class VirtualHostEntryUpgrader
addAttributeTransformer("jdbcBytesForBlob",
addContextVar("qpid.jdbcstore.useBytesForBlob")).
addAttributeTransformer("jdbcBlobType",
addContextVar("qpid.jdbcstore.blobType")).
addAttributeTransformer("jdbcVarbinaryType",
addContextVar("qpid.jdbcstore.varBinaryType")).
-addAttributeTransformer("partitionCount",
addContextVar("qpid.jdbcstore.bonecp.partitionCount")).
-addAttributeTransformer("maxConnectionsPerPartition",
-
addContextVar("qpid.jdbcstore.bonecp.maxConnectionsPerPartition")).
-addAttributeTransformer("minConnectionsPerPartition",
-
addContextVar("qpid.jdbcstore.bonecp.minConnectionsPerPartition")),
+addAttributeTransformer("maximumPoolSize",
+
addContextVar("qpid.jdbcstore.hikaricp.maximumPoolSize")).
+addAttributeTransformer("minimumIdle",
+
addContextVar("qpid.jdbcstore.hikaricp.minimumIdle")),
Review Comment:
I'm not 100% sure, its a long time since I really contributed to the broker
and things changed since then. Originally when we added that, I believe it was
just its own independent config version. From
broker-core/src/main/java/org/apache/qpid/server/model/BrokerModel.java it
looks like around the 6.0.0 release (when the release version was bumped as
everything became an independent component, rather than the big 'qpid release'
with everything) that the model version was also jumped to bring it into 'major
6, minor 0' alignment, and since then it seems like it has consistently had
changes that mean the major matched the broker release major version. It looks
like maybe the minors too. Perhaps digging more at the changes that have been
made historically will make it clearer for you what would be best, but using
either 9.1.0 (next minor) or 9.2.0 (matching minor) seems fair if the release
version is 9.2.0.
--
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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
dakirily commented on code in PR #235:
URL: https://github.com/apache/qpid-broker-j/pull/235#discussion_r1474043869
##
broker-core/src/main/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecoverer.java:
##
@@ -805,11 +805,10 @@ private static class VirtualHostEntryUpgrader
addAttributeTransformer("jdbcBytesForBlob",
addContextVar("qpid.jdbcstore.useBytesForBlob")).
addAttributeTransformer("jdbcBlobType",
addContextVar("qpid.jdbcstore.blobType")).
addAttributeTransformer("jdbcVarbinaryType",
addContextVar("qpid.jdbcstore.varBinaryType")).
-addAttributeTransformer("partitionCount",
addContextVar("qpid.jdbcstore.bonecp.partitionCount")).
-addAttributeTransformer("maxConnectionsPerPartition",
-
addContextVar("qpid.jdbcstore.bonecp.maxConnectionsPerPartition")).
-addAttributeTransformer("minConnectionsPerPartition",
-
addContextVar("qpid.jdbcstore.bonecp.minConnectionsPerPartition")),
+addAttributeTransformer("maximumPoolSize",
+
addContextVar("qpid.jdbcstore.hikaricp.maximumPoolSize")).
+addAttributeTransformer("minimumIdle",
+
addContextVar("qpid.jdbcstore.hikaricp.minimumIdle")),
Review Comment:
Hi Robbie,
Thank you for the review. You're right, that needs to be fixed.
Are there any conventions regarding the model version change? It seems to me
that as only the context attribute names / values were changed without adding
new entities or attributes, model version could be changed from 9.0 to 9.1? Or
should the model version match the broker version as well? E.g. model version
9.2 for broker version 9.2.0 until other model change or if model changes to 10
then qpid-broker-j version have to be changed 10.0.0 as well?
--
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
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
gemmellr commented on code in PR #235:
URL: https://github.com/apache/qpid-broker-j/pull/235#discussion_r1473110588
##
broker-core/src/main/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecoverer.java:
##
@@ -805,11 +805,10 @@ private static class VirtualHostEntryUpgrader
addAttributeTransformer("jdbcBytesForBlob",
addContextVar("qpid.jdbcstore.useBytesForBlob")).
addAttributeTransformer("jdbcBlobType",
addContextVar("qpid.jdbcstore.blobType")).
addAttributeTransformer("jdbcVarbinaryType",
addContextVar("qpid.jdbcstore.varBinaryType")).
-addAttributeTransformer("partitionCount",
addContextVar("qpid.jdbcstore.bonecp.partitionCount")).
-addAttributeTransformer("maxConnectionsPerPartition",
-
addContextVar("qpid.jdbcstore.bonecp.maxConnectionsPerPartition")).
-addAttributeTransformer("minConnectionsPerPartition",
-
addContextVar("qpid.jdbcstore.bonecp.minConnectionsPerPartition")),
+addAttributeTransformer("maximumPoolSize",
+
addContextVar("qpid.jdbcstore.hikaricp.maximumPoolSize")).
+addAttributeTransformer("minimumIdle",
+
addContextVar("qpid.jdbcstore.hikaricp.minimumIdle")),
Review Comment:
I dont understand the changes here. These alterations were made to an
old/previous config model upgrade step. Altering that changes the behaviour of
the old upgrade (and potentially of the later model upgrades that followed it,
that might depend on the expected prior behaviour), to instead partly reflect
_current_ config. Config that didnt even exist at the point/model-version that
particular upgrade step is for (and again, later ones). That just doesnt make
sense to me.
At the same time, there also doesnt actually look to be any _new_ upgrade
step introduced to similarly handle the pool having changed entirely from 9.1.0
to 9.2.0, i.e within a minor release for the 9.x model version config such
recent brokers will actually have. The tests that were altered are still
indicating feeding in model version 0.4 config from years ago (yet have changed
the input so it isnt 0.4 config). Where are the additional checks for behaviour
handling and/or actual-update of model version 9.x config from recent brokers?
I honestly cant tell what the expected behaviour is here for an upgrading
user with this change. The point of these upgrader bits was to auto-update
prior version config (in version specific stages) as appropriate to keep things
working with the newer broker, such that starting a new e.g 9.2.0 broker with
old e.g 9.1.0 broker config results in new-broker config that will still work.
Is this meant to be doing that (as it partly is here, though in an old place it
seems like it shouldnt be changing)? If so, I'm not seeing where it really does
that fully, and I'm not seeing appropriate tests to verify it really does that.
If its not expected to do that update, then I dont see why any change was made
here at all, and it seems more like a major version change is needed.
##
broker-core/src/test/java/org/apache/qpid/server/store/BrokerStoreUpgraderAndRecovererTest.java:
##
@@ -90,18 +90,17 @@ public void setUp() throws Exception
}
@Test
-public void testUpgradeVirtualHostWithJDBCStoreAndBoneCPPool()
+public void testUpgradeVirtualHostWithJDBCStoreAndHikariCPPool()
{
final Map hostAttributes = ImmutableMap.builder()
.put("name", VIRTUALHOST_NAME)
.put("modelVersion", "0.4")
-.put("connectionPool", "BONECP")
+.put("connectionPool", "HIKARICP")
.put("connectionURL",
"jdbc:derby://localhost:1527/tmp/vh/test;create=true")
.put("createdBy", VIRTUALHOST_CREATED_BY)
.put("createdTime", VIRTUALHOST_CREATE_TIME)
-.put("maxConnectionsPerPartition", 7)
-.put("minConnectionsPerPartition", 6)
-.put("partitionCount", 2)
+.put("maximumPoolSize", 7)
+.put("minimumIdle", 6)
Review Comment:
This (and bit above) does not make sense to me. This test was almost a
decade old and is apparently meant to reflect model 0.4 config from the time
being updated in some way, and show whats expected actually happens when that
old config is fed in and run through the current updater. There was no such
config as this for the old broker model version update this test was for, so it
doesnt make sense to change the input like this - its now no longer testing
that the update for that model version transition handled what it was supposed
to / has done for the last decade.
The ultimate output might
Re: [PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
vavrtom merged PR #235: URL: https://github.com/apache/qpid-broker-j/pull/235 -- 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
[PR] QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement [qpid-broker-j]
dakirily opened a new pull request, #235: URL: https://github.com/apache/qpid-broker-j/pull/235 This PR addresses JIRA [QPID-8666](https://issues.apache.org/jira/browse/QPID-8666), replacing broker plugin jdbc-provider-bone with the new one jdbc-provider-hikaricp based on HikariCP -- 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
