QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement #239
Conversation
| final ConfiguredObjectRecord systemConfigRecord = mock(ConfiguredObjectRecord.class);; | ||
| when(systemConfigRecord.getId()).thenReturn(randomUUID()); | ||
| when(systemConfigRecord.getType()).thenReturn("SystemConfig"); | ||
| when(systemConfigRecord.getAttributes()).thenReturn(Map.copyOf(attributes)); |
There was a problem hiding this 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?
There was a problem hiding this comment.
The test was fixed to use a copy of a context map for each object.
| final ConfiguredObjectRecord virtualHostLoggerRecord = mock(ConfiguredObjectRecord.class);; | ||
| when(virtualHostLoggerRecord.getId()).thenReturn(randomUUID()); | ||
| when(virtualHostLoggerRecord.getType()).thenReturn("VirtualHostLogger"); | ||
| when(virtualHostLoggerRecord.getAttributes()).thenReturn(attributes); |
There was a problem hiding this 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.
There was a problem hiding this comment.
The test was fixed in the same way as in BrokerStoreUpgraderAndRecovererTest to keep to uniformity.
QPID-8666: [Broker-J] Broker plugin jdbc-provider-bone replacement
There was a problem hiding this comment.
The change from prior feedback looks good.
I noticed something I didnt earlier when considering it in the context of the new changes.
I was then also looking at the earlier changes again and wonder about the apparent different naming and values between the java code and the web ui code. Its not clear to me they are consistent. Are they meant to differ in the ways they do? E.g:
| 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())) |
There was a problem hiding this 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?)
There was a problem hiding this comment.
Variable name were renamed appropriately.
The upgrader code was changed to make logic for BONECP old type and "Broker" entity the same.
This PR is a follow-up to the PR-235 and PR-238 and addresses JIRA QPID-8666, adding upgrading the broker model version from 9.0 to 9.1