QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement #235
Conversation
| addAttributeTransformer("maximumPoolSize", | ||
| addContextVar("qpid.jdbcstore.hikaricp.maximumPoolSize")). | ||
| addAttributeTransformer("minimumIdle", | ||
| addContextVar("qpid.jdbcstore.hikaricp.minimumIdle")), |
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this 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.
| .put("maximumPoolSize", 7) | ||
| .put("minimumIdle", 6) |
There was a problem hiding this 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 PR addresses JIRA QPID-8666, replacing broker plugin jdbc-provider-bone with the new one jdbc-provider-hikaricp based on HikariCP