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<String, Object> hostAttributes = ImmutableMap.<String,
Object>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 change, because of the latest model version
updates further changing it, but the input and the intermediate updates
shouldn't be changing for previously-released updates, especially not in ways
that simply never occurred in the model version config of that time.
--
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
