ARTEMIS-813 Ensure no duplicate journal records on address update
Project: http://git-wip-us.apache.org/repos/asf/activemq-artemis/repo Commit: http://git-wip-us.apache.org/repos/asf/activemq-artemis/commit/af277140 Tree: http://git-wip-us.apache.org/repos/asf/activemq-artemis/tree/af277140 Diff: http://git-wip-us.apache.org/repos/asf/activemq-artemis/diff/af277140 Branch: refs/heads/master Commit: af277140264ea741996dcb8aa270dd99992b3f5d Parents: 6ab133a Author: jbertram <[email protected]> Authored: Mon Dec 5 20:40:46 2016 -0600 Committer: Martyn Taylor <[email protected]> Committed: Fri Dec 9 18:43:15 2016 +0000 ---------------------------------------------------------------------- .../artemis/core/postoffice/AddressManager.java | 12 ++++- .../artemis/core/postoffice/PostOffice.java | 12 ++++- .../core/postoffice/impl/PostOfficeImpl.java | 32 +++++++----- .../postoffice/impl/SimpleAddressManager.java | 30 +++++------- .../artemis/core/server/ActiveMQServer.java | 6 +-- .../core/server/impl/ActiveMQServerImpl.java | 51 ++++++++------------ .../server/impl/PostOfficeJournalLoader.java | 6 +-- .../core/server/impl/ServerSessionImpl.java | 6 ++- .../core/server/impl/fakes/FakePostOffice.java | 8 +-- 9 files changed, 84 insertions(+), 79 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/AddressManager.java ---------------------------------------------------------------------- diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/AddressManager.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/AddressManager.java index 6ba205b..a5a1109 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/AddressManager.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/AddressManager.java @@ -58,12 +58,20 @@ public interface AddressManager { Set<SimpleString> getAddresses(); - AddressInfo addAddressInfo(AddressInfo addressInfo); + /** + * @param addressInfo + * @return true if the address was added, false if it wasn't added + */ + boolean addAddressInfo(AddressInfo addressInfo); AddressInfo updateAddressInfoIfPresent(SimpleString addressName, BiFunction<? super SimpleString, ? super AddressInfo, ? extends AddressInfo> remappingFunction); - AddressInfo addOrUpdateAddressInfo(AddressInfo addressInfo); + /** + * @param addressInfo + * @return true if the address was added, false if it was updated + */ + boolean addOrUpdateAddressInfo(AddressInfo addressInfo); AddressInfo removeAddressInfo(SimpleString address); http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/PostOffice.java ---------------------------------------------------------------------- diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/PostOffice.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/PostOffice.java index 3c40475..cb787c7 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/PostOffice.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/PostOffice.java @@ -45,9 +45,17 @@ import org.apache.activemq.artemis.core.transaction.Transaction; */ public interface PostOffice extends ActiveMQComponent { - AddressInfo addAddressInfo(AddressInfo addressInfo); + /** + * @param addressInfo + * @return true if the address was added, false if it wasn't added + */ + boolean addAddressInfo(AddressInfo addressInfo); - AddressInfo addOrUpdateAddressInfo(AddressInfo addressInfo); + /** + * @param addressInfo + * @return true if the address was added, false if it was updated + */ + boolean addOrUpdateAddressInfo(AddressInfo addressInfo); AddressInfo removeAddressInfo(SimpleString address) throws Exception; http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java ---------------------------------------------------------------------- diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java index c7df757..69256f2 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java @@ -424,26 +424,34 @@ public class PostOfficeImpl implements PostOffice, NotificationListener, Binding // PostOffice implementation ----------------------------------------------- @Override - public AddressInfo addAddressInfo(AddressInfo addressInfo) { + public boolean addAddressInfo(AddressInfo addressInfo) { synchronized (addressLock) { - try { - managementService.registerAddress(addressInfo); - } catch (Exception e) { - e.printStackTrace(); + boolean result = addressManager.addAddressInfo(addressInfo); + // only register address if it is new + if (result) { + try { + managementService.registerAddress(addressInfo); + } catch (Exception e) { + e.printStackTrace(); + } } - return addressManager.addAddressInfo(addressInfo); + return result; } } @Override - public AddressInfo addOrUpdateAddressInfo(AddressInfo addressInfo) { + public boolean addOrUpdateAddressInfo(AddressInfo addressInfo) { synchronized (addressLock) { - try { - managementService.registerAddress(addressInfo); - } catch (Exception e) { - e.printStackTrace(); + boolean result = addressManager.addOrUpdateAddressInfo(addressInfo); + // only register address if it is newly added + if (result) { + try { + managementService.registerAddress(addressInfo); + } catch (Exception e) { + e.printStackTrace(); + } } - return addressManager.addOrUpdateAddressInfo(addressInfo); + return result; } } http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java ---------------------------------------------------------------------- diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java index 59f285c..0ae9c82 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java @@ -30,7 +30,6 @@ import org.apache.activemq.artemis.core.postoffice.Binding; import org.apache.activemq.artemis.core.postoffice.Bindings; import org.apache.activemq.artemis.core.postoffice.BindingsFactory; import org.apache.activemq.artemis.core.server.ActiveMQMessageBundle; -import org.apache.activemq.artemis.core.server.ActiveMQServerLogger; import org.apache.activemq.artemis.core.server.RoutingType; import org.apache.activemq.artemis.core.server.impl.AddressInfo; import org.apache.activemq.artemis.core.transaction.Transaction; @@ -216,8 +215,8 @@ public class SimpleAddressManager implements AddressManager { } @Override - public AddressInfo addAddressInfo(AddressInfo addressInfo) { - return addressInfoMap.putIfAbsent(addressInfo.getName(), addressInfo); + public boolean addAddressInfo(AddressInfo addressInfo) { + return addressInfoMap.putIfAbsent(addressInfo.getName(), addressInfo) == null; } @Override @@ -226,23 +225,20 @@ public class SimpleAddressManager implements AddressManager { return addressInfoMap.computeIfPresent(addressName, remappingFunction); } - @Override - public AddressInfo addOrUpdateAddressInfo(AddressInfo addressInfo) { - AddressInfo from = addAddressInfo(addressInfo); - if (from != null) { - ActiveMQServerLogger.LOGGER.info("Address " + addressInfo.getName() + " exists already as " + from + ", updating instead with: " + addressInfo); - } - return (from == null) ? addressInfo : updateAddressInfo(from, addressInfo); - } + public boolean addOrUpdateAddressInfo(AddressInfo addressInfo) { + boolean isNew = addAddressInfo(addressInfo); - private AddressInfo updateAddressInfo(AddressInfo from, AddressInfo to) { - synchronized (from) { - for (RoutingType routingType : to.getRoutingTypes()) { - from.addRoutingType(routingType); + // address already exists so update it + if (!isNew) { + AddressInfo toUpdate = getAddressInfo(addressInfo.getName()); + synchronized (toUpdate) { + for (RoutingType routingType : addressInfo.getRoutingTypes()) { + toUpdate.addRoutingType(routingType); + } } - ActiveMQServerLogger.LOGGER.info("Update result: " + from); - return from; } + + return isNew; } @Override http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java ---------------------------------------------------------------------- diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java index 65d256a..f90a697 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java @@ -474,11 +474,9 @@ public interface ActiveMQServer extends ActiveMQComponent { */ void removeRoutingType(String address, RoutingType routingType) throws Exception; - AddressInfo putAddressInfoIfAbsent(AddressInfo addressInfo) throws Exception; + boolean createAddressInfo(AddressInfo addressInfo) throws Exception; - void createAddressInfo(AddressInfo addressInfo) throws Exception; - - AddressInfo createOrUpdateAddressInfo(AddressInfo addressInfo) throws Exception; + boolean createOrUpdateAddressInfo(AddressInfo addressInfo) throws Exception; void removeAddressInfo(SimpleString address, SecurityAuth session) throws Exception; http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---------------------------------------------------------------------- diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java index 64fe2b7..6d30a28 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java @@ -2198,16 +2198,6 @@ public class ActiveMQServerImpl implements ActiveMQServer { // Deploy any predefined queues deployQueuesFromConfiguration(); - // registerPostQueueDeletionCallback(new PostQueueDeletionCallback() { - // // TODO delete auto-created addresses when queueCount == 0 - // @Override - // public void callback(SimpleString address, SimpleString queueName) throws Exception { - // if (getAddressInfo(address).isAutoCreated()) { - // removeAddressInfo(address); - // } - // } - // }); - // We need to call this here, this gives any dependent server a chance to deploy its own addresses // this needs to be done before clustering is fully activated callActivateCallbacks(); @@ -2408,34 +2398,34 @@ public class ActiveMQServerImpl implements ActiveMQServer { postOffice.removeRoutingType(addressName,routingType); } - @Override - public AddressInfo putAddressInfoIfAbsent(AddressInfo addressInfo) throws Exception { - AddressInfo result = postOffice.addAddressInfo(addressInfo); - - // TODO: is this the right way to do this? - long txID = storageManager.generateID(); - storageManager.addAddressBinding(txID, addressInfo); - storageManager.commitBindings(txID); - - return result; - } + public boolean createAddressInfo(AddressInfo addressInfo) throws Exception { + boolean result = postOffice.addAddressInfo(addressInfo); - @Override - public void createAddressInfo(AddressInfo addressInfo) throws Exception { - if (putAddressInfoIfAbsent(addressInfo) != null) { + if (result) { + long txID = storageManager.generateID(); + storageManager.addAddressBinding(txID, addressInfo); + storageManager.commitBindings(txID); + } else { throw ActiveMQMessageBundle.BUNDLE.addressAlreadyExists(addressInfo.getName()); } + + return result; } @Override - public AddressInfo createOrUpdateAddressInfo(AddressInfo addressInfo) throws Exception { - AddressInfo result = postOffice.addOrUpdateAddressInfo(addressInfo); + public boolean createOrUpdateAddressInfo(AddressInfo addressInfo) throws Exception { + boolean result = postOffice.addOrUpdateAddressInfo(addressInfo); - // TODO: is this the right way to do this? - // TODO: deal with possible duplicates, may be adding new records when old ones already exist long txID = storageManager.generateID(); - storageManager.addAddressBinding(txID, addressInfo); - storageManager.commitBindings(txID); + if (result) { + storageManager.addAddressBinding(txID, addressInfo); + storageManager.commitBindings(txID); + } else { + AddressInfo updatedAddressInfo = getAddressInfo(addressInfo.getName()); + storageManager.deleteAddressBinding(txID, updatedAddressInfo.getId()); + storageManager.addAddressBinding(txID, updatedAddressInfo); + storageManager.commitBindings(txID); + } return result; } @@ -2452,7 +2442,6 @@ public class ActiveMQServerImpl implements ActiveMQServer { throw ActiveMQMessageBundle.BUNDLE.addressDoesNotExist(address); } - // TODO: is this the right way to do this? Should it use a transaction? long txID = storageManager.generateID(); storageManager.deleteAddressBinding(txID, addressInfo.getId()); storageManager.commitBindings(txID); http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/PostOfficeJournalLoader.java ---------------------------------------------------------------------- diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/PostOfficeJournalLoader.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/PostOfficeJournalLoader.java index f52b5cc..9c4c617 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/PostOfficeJournalLoader.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/PostOfficeJournalLoader.java @@ -166,7 +166,6 @@ public class PostOfficeJournalLoader implements JournalLoader { queues.put(queue.getID(), queue); postOffice.addBinding(binding); - //managementService.registerAddress(queue.getAddress()); managementService.registerQueue(queue, queue.getAddress(), storageManager); } @@ -178,11 +177,8 @@ public class PostOfficeJournalLoader implements JournalLoader { for (AddressBindingInfo addressBindingInfo : addressBindingInfos) { addressBindingInfosMap.put(addressBindingInfo.getId(), addressBindingInfo); - // TODO: figure out what else to set here - AddressInfo addressInfo = new AddressInfo(addressBindingInfo.getName()) - .setRoutingTypes(addressBindingInfo.getRoutingTypes()); + AddressInfo addressInfo = new AddressInfo(addressBindingInfo.getName()).setRoutingTypes(addressBindingInfo.getRoutingTypes()); postOffice.addAddressInfo(addressInfo); - managementService.registerAddress(addressInfo); } } http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java ---------------------------------------------------------------------- diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java index dec6f65..4d0985b 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java @@ -585,7 +585,8 @@ public class ServerSessionImpl implements ServerSession, FailureListener { final boolean autoCreated) throws Exception { Pair<SimpleString, Set<RoutingType>> art = getAddressAndRoutingTypes(address, routingTypes); securityCheck(art.getA(), CheckType.CREATE_ADDRESS, this); - return server.createOrUpdateAddressInfo(new AddressInfo(art.getA(), art.getB()).setAutoCreated(autoCreated)); + server.createOrUpdateAddressInfo(new AddressInfo(art.getA(), art.getB()).setAutoCreated(autoCreated)); + return server.getAddressInfo(art.getA()); } @Override @@ -594,7 +595,8 @@ public class ServerSessionImpl implements ServerSession, FailureListener { final boolean autoCreated) throws Exception { Pair<SimpleString, RoutingType> art = getAddressAndRoutingType(address, routingType); securityCheck(art.getA(), CheckType.CREATE_ADDRESS, this); - return server.createOrUpdateAddressInfo(new AddressInfo(art.getA(), art.getB()).setAutoCreated(autoCreated)); + server.createOrUpdateAddressInfo(new AddressInfo(art.getA(), art.getB()).setAutoCreated(autoCreated)); + return server.getAddressInfo(art.getA()); } @Override http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/af277140/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/server/impl/fakes/FakePostOffice.java ---------------------------------------------------------------------- diff --git a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/server/impl/fakes/FakePostOffice.java b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/server/impl/fakes/FakePostOffice.java index a93cc3c..dc5fedf 100644 --- a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/server/impl/fakes/FakePostOffice.java +++ b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/server/impl/fakes/FakePostOffice.java @@ -85,13 +85,13 @@ public class FakePostOffice implements PostOffice { } @Override - public AddressInfo addAddressInfo(AddressInfo addressInfo) { - return null; + public boolean addAddressInfo(AddressInfo addressInfo) { + return false; } @Override - public AddressInfo addOrUpdateAddressInfo(AddressInfo addressInfo) { - return null; + public boolean addOrUpdateAddressInfo(AddressInfo addressInfo) { + return false; }
