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