ARTEMIS-4366 Missing Mirrored ACKs with MULTICAST and subscriptions #4555
Conversation
clebertsuconic
force-pushed
the
mirrored-acks
branch
8 times, most recently
from
bf88fce
to
bdcb5fb
Compare
clebertsuconic
force-pushed
the
mirrored-acks
branch
from
bdcb5fb
to
2f73c69
Compare
| @@ -75,7 +75,7 @@ public static String getMirrorAddress(String connectionName) { | |||
|
|
|||
| // We must use one referenceIDSupplier per server. | |||
| // protocol manager is the perfect aggregation for that. | |||
| private ReferenceNodeStore referenceIDSupplier; | |||
| private ReferenceNodeStoreFactory referenceIDSupplier; | |||
There was a problem hiding this comment.
Change makes the comment above seem inaccurate / contradictory (somewhat similar with the field name). There is now a factory being returned, and so the overall change here seems like it was to make sure there is not a single reference ID supplier per server as the comment says, but rather now one per-queue.
| public ReferenceNodeStoreFactory(ActiveMQServer server) { | ||
| this.server = server; | ||
| this.serverID = server.getNodeID().toString(); | ||
|
|
| return (Long)message.getBrokerProperty(INTERNAL_ID_EXTRA_PROPERTY); | ||
| } | ||
|
|
||
|
|
| return getServerID(element.getMessage()); | ||
| } | ||
|
|
||
|
|
| public class ReferenceNodeStore implements NodeStore<MessageReference> { | ||
|
|
||
| private final String serverID; | ||
| private final ReferenceNodeStoreFactory factory; |
There was a problem hiding this comment.
It seems especially weird for a ReferenceNodeStore to contain the ReferenceNodeStoreFactory that creates ReferenceNodeStore. One which it then only uses to get IDs (which are then really just taken from the message [reference]).
Feels like most or all this usage should be a different interface/ object.
| @@ -125,11 +125,11 @@ public ProtonProtocolManager(ProtonProtocolManagerFactory factory, ActiveMQServe | |||
| routingHandler = new AMQPRoutingHandler(server); | |||
| } | |||
|
|
|||
| public synchronized ReferenceNodeStore getReferenceIDSupplier() { | |||
| public synchronized ReferenceNodeStoreFactory getReferenceIDSupplier() { | |||
There was a problem hiding this comment.
The method name also seems off now.
There was a problem hiding this comment.
It still the ID supplier.
That's a reason why I actually didn't make those methods static. although one of them can't be made static.
I can rename this as IDSupplier. it would implement the NodeStoreFactory and return a new instance of NodeStore.
There was a problem hiding this comment.
Think the only one that wouldnt go static is getDefaultNodeID() , which didnt originally exist as the caller already had the value (serverId).
Renaming would be good...the current naming just doesnt fit, makes it confusing to read.
|
Its odd/confusing that the ReferenceNodeStoreFactory is still named to in most parameters and fields as 'referenceIDSupplier' or 'nodeStore', especially the latter with there being an actual seperate ReferenceNodeStore. Its mixing roles and field/param names up in a confusing manner. Feels like the behaviours should be split into separate concerns, as they largely were originally. Alternatively many of the ID methods now on ReferenceNodeStoreFactory could perhaps just be made static helpers rather than being 'passed around on the side of a factory', since they only pull details from the message[reference] rather than the factory itself. |
This is in relation to comments from PR apache#4555
This is in relation to comments from PR #4555
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30)
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
This is in relation to comments from PR apache#4555 (cherry picked from commit 29deb30) downstream: ENTMQBR-8220
No description provided.