Author: chris.laprun(a)jboss.com Date: 2012-03-12 15:37:41 -0400 (Mon, 12 Mar 2012) New Revision: 8573 Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/ProducerInfo.java components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationInfo.java components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/registry/AbstractConsumerRegistry.java components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/registry/InMemoryConsumerRegistry.java components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/ProducerInfoTestCase.java Log: - GTNWSRP-279: Avoid updating ProducerInfo when not needed and added changes detection. Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/ProducerInfo.java =================================================================== --- components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/ProducerInfo.java 2012-03-12 19:37:32 UTC (rev 8572) +++ components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/ProducerInfo.java 2012-03-12 19:37:41 UTC (rev 8573) @@ -1,6 +1,6 @@ /* * JBoss, a division of Red Hat - * Copyright 2011, Red Hat Middleware, LLC, and individual + * Copyright 2012, Red Hat Middleware, LLC, and individual * contributors as indicated by the @authors tag. See the * copyright.txt in the distribution for a full listing of * individual contributors. @@ -107,7 +107,6 @@ /** The activated status of the associated Consumer */ private boolean persistentActive; - // GTNWSRP-239: information that's currently transient but should probably be persistent /** * GTNWSRP-239: whether or not this ProducerInfo requires ModifyRegistration to be called, currently persisted via * mixin @@ -180,7 +179,7 @@ { return false; } - if (!persistentId.equals(that.persistentId)) + if (!getId().equals(that.getId())) { return false; } @@ -192,7 +191,7 @@ public int hashCode() { int result = key != null ? key.hashCode() : 0; - result = 31 * result + persistentId.hashCode(); + result = 31 * result + getId().hashCode(); return result; } @@ -202,7 +201,7 @@ final StringBuilder sb = new StringBuilder(); sb.append("ProducerInfo"); sb.append("{key='").append(key).append('\''); - sb.append(", id='").append(persistentId).append('\''); + sb.append(", id='").append(getId()).append('\''); sb.append('}'); return sb.toString(); } @@ -302,13 +301,46 @@ */ public void setActive(boolean active) { + setInternalActive(active); + } + + private boolean setInternalActive(boolean active) + { + final boolean modified = modifyNowIfNeeded(persistentActive, active); this.persistentActive = active; + return modified; } + public String getId() + { + return persistentId; + } + + public void setId(String id) + { + modifyNowIfNeeded(persistentId, id); + this.persistentId = id; + } + + private boolean modifyNowIfNeeded(Object oldValue, Object newValue) + { + if (ParameterValidation.isOldAndNewDifferent(oldValue, newValue)) + { + modifyNow(); + return true; + } + else + { + return false; + } + } + public void setActiveAndSave(boolean active) { - setActive(active); - registry.updateProducerInfo(this); + if (setInternalActive(active)) + { + registry.updateProducerInfo(this); + } } public boolean isModifyRegistrationRequired() @@ -320,6 +352,7 @@ public void setModifyRegistrationRequired(boolean modifyRegistrationRequired) { + modifyNowIfNeeded(isModifyRegistrationRequired, modifyRegistrationRequired); this.isModifyRegistrationRequired = modifyRegistrationRequired; } @@ -574,16 +607,16 @@ } result.setRegistrationResult(registrationResult); - - return result; } else { log.debug("Registration not required"); persistentRegistrationInfo = new RegistrationInfo(this, false); extractOfferedPortlets(serviceDescription); - return result; } + + modifyNow(); + return result; } private Map<String, ItemDescription> toMap(List<ItemDescription> itemDescriptions) @@ -603,16 +636,6 @@ } } - public String getId() - { - return persistentId; - } - - public void setId(String id) - { - this.persistentId = id; - } - /** * Extracts a map of offered Portlet objects from ServiceDescription * @@ -673,7 +696,7 @@ { log.warn("Portlet '" + portletHandle + "' uses the GET method in forms. Since we don't handle this, this portlet will be excluded from " + - "the list of offered portlets for producer " + persistentId); + "the list of offered portlets for producer " + getId()); } else { @@ -749,7 +772,7 @@ } catch (Exception e) { - log.debug("Couldn't get portlet via getPortletDescription for producer '" + persistentId + log.debug("Couldn't get portlet via getPortletDescription for producer '" + getId() + "'. Attempting to retrieve it from the service description as this producer might not support the PortletManagement interface.", e); justRefreshed = refresh(true); @@ -831,24 +854,26 @@ public void setExpirationCacheSeconds(Integer expirationCacheSeconds) { - // record the previous cache expiration duration - Integer previousMS = getSafeExpirationCacheSeconds() * 1000; + if (modifyNowIfNeeded(persistentExpirationCacheSeconds, expirationCacheSeconds)) + { + // record the previous cache expiration duration + Integer previousMS = getSafeExpirationCacheSeconds() * 1000; - // assign the new value - this.persistentExpirationCacheSeconds = expirationCacheSeconds; + // assign the new value + this.persistentExpirationCacheSeconds = expirationCacheSeconds; - // recompute the expiration time based on previous value and new one - long lastExpirationTimeChange = expirationTimeMillis - previousMS; - int newMS = getSafeExpirationCacheSeconds() * 1000; - if (lastExpirationTimeChange > 0) - { - expirationTimeMillis = lastExpirationTimeChange + newMS; + // recompute the expiration time based on previous value and new one + long lastExpirationTimeChange = expirationTimeMillis - previousMS; + int newMS = getSafeExpirationCacheSeconds() * 1000; + if (lastExpirationTimeChange > 0) + { + expirationTimeMillis = lastExpirationTimeChange + newMS; + } + else + { + expirationTimeMillis = System.currentTimeMillis(); + } } - else - { - expirationTimeMillis = System.currentTimeMillis(); - } - } /** @@ -1006,7 +1031,7 @@ { Throwable cause = e.getCause(); throw new InvokerUnavailableException("Problem getting service description for producer " - + persistentId + ", please see the logs for more information. ", cause == null ? e : cause); + + getId() + ", please see the logs for more information. ", cause == null ? e : cause); } public RegistrationContext getRegistrationContext() throws PortletInvokerException @@ -1024,9 +1049,15 @@ persistentRegistrationInfo.resetRegistration(); invalidateCache(); + modifyNow(); registry.updateProducerInfo(this); } + void modifyNow() + { + setLastModified(System.nanoTime()); + } + // make package only after package reorg public PortletPropertyDescriptionResponse getPropertyDescriptionsFor(String portletHandle) @@ -1116,7 +1147,7 @@ if (serviceDescription.isRequiresRegistration()) { // check if the configured registration information is correct and if we can get the service description - RefreshResult result = persistentRegistrationInfo.refresh(serviceDescription, persistentId, true, forceRefresh, false); + RefreshResult result = persistentRegistrationInfo.refresh(serviceDescription, getId(), true, forceRefresh, false); if (!result.hasIssues()) { try @@ -1144,7 +1175,7 @@ if (debug) { - String msg = "Consumer with id '" + persistentId + "' successfully registered with handle: '" + String msg = "Consumer with id '" + getId() + "' successfully registered with handle: '" + registrationContext.getRegistrationHandle() + "'"; log.debug(msg); } @@ -1158,7 +1189,7 @@ { persistentRegistrationInfo.resetRegistration(); setActive(false); - throw new PortletInvokerException("Couldn't register with producer '" + persistentId + "'", e); + throw new PortletInvokerException("Couldn't register with producer '" + getId() + "'", e); } } else @@ -1183,11 +1214,11 @@ { RegistrationContext registrationContext = getRegistrationContext(); persistentEndpointInfo.getRegistrationService().deregister(registrationContext, UserAccess.getUserContext()); - log.info("Consumer with id '" + persistentId + "' deregistered."); + log.info("Consumer with id '" + getId() + "' deregistered."); } catch (Exception e) { - throw new PortletInvokerException("Couldn't deregister with producer '" + persistentId + "'", e); + throw new PortletInvokerException("Couldn't deregister with producer '" + getId() + "'", e); } finally { @@ -1196,7 +1227,7 @@ } else { - throw new IllegalStateException("Cannot deregister producer '" + persistentId + "' as it's not registered"); + throw new IllegalStateException("Cannot deregister producer '" + getId() + "' as it's not registered"); } } @@ -1244,20 +1275,20 @@ // update state persistentRegistrationInfo.setRegistrationState(registrationState.value); - log.info("Consumer with id '" + persistentId + "' sucessfully modified its registration."); + log.info("Consumer with id '" + getId() + "' sucessfully modified its registration."); // reset cache to be able to see new offered portlets on the next refresh invalidateCache(); } catch (Exception e) { - throw new PortletInvokerException("Couldn't modify registration with producer '" + persistentId + "'", e); + throw new PortletInvokerException("Couldn't modify registration with producer '" + getId() + "'", e); } } } else { - throw new IllegalStateException("Cannot modify registration for producer '" + persistentId + throw new IllegalStateException("Cannot modify registration for producer '" + getId() + "' as it's not registered"); } } @@ -1273,9 +1304,9 @@ private RefreshResult internalRefreshRegistration(ServiceDescription serviceDescription, boolean mergeWithLocalInfo, boolean forceRefresh, boolean forceCheckOfExtraProps) throws PortletInvokerException { RefreshResult result = - persistentRegistrationInfo.refresh(serviceDescription, persistentId, mergeWithLocalInfo, forceRefresh, forceCheckOfExtraProps); + persistentRegistrationInfo.refresh(serviceDescription, getId(), mergeWithLocalInfo, forceRefresh, forceCheckOfExtraProps); - log.debug("Refreshed registration information for consumer with id '" + persistentId + "'"); + log.debug("Refreshed registration information for consumer with id '" + getId() + "'"); return result; } @@ -1287,7 +1318,7 @@ || persistentEndpointInfo.isRefreshNeeded(); if (result) { - log.debug("Refresh needed for producer '" + persistentId + "'"); + log.debug("Refresh needed for producer '" + getId() + "'"); } return result; } Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationInfo.java =================================================================== --- components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationInfo.java 2012-03-12 19:37:32 UTC (rev 8572) +++ components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationInfo.java 2012-03-12 19:37:41 UTC (rev 8573) @@ -1,6 +1,6 @@ /* * JBoss, a division of Red Hat - * Copyright 2011, Red Hat Middleware, LLC, and individual + * Copyright 2012, Red Hat Middleware, LLC, and individual * contributors as indicated by the @authors tag. See the * copyright.txt in the distribution for a full listing of * individual contributors. @@ -745,6 +745,9 @@ } regenerateRegistrationData = true; + + // make sure that the parent is marked as modified so that changes can be properly saved + parent.modifyNow(); } private void setModifyRegistrationNeeded(boolean modifyRegistrationNeeded) Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/registry/AbstractConsumerRegistry.java =================================================================== --- components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/registry/AbstractConsumerRegistry.java 2012-03-12 19:37:32 UTC (rev 8572) +++ components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/registry/AbstractConsumerRegistry.java 2012-03-12 19:37:41 UTC (rev 8573) @@ -1,6 +1,6 @@ /* * JBoss, a division of Red Hat - * Copyright 2011, Red Hat Middleware, LLC, and individual + * Copyright 2012, Red Hat Middleware, LLC, and individual * contributors as indicated by the @authors tag. See the * copyright.txt in the distribution for a full listing of * individual contributors. @@ -65,7 +65,7 @@ private static final String CONSUMER_WITH_ID = "Consumer with id '"; private static final String RELEASE_SESSIONS_LISTENER = "release_sessions_listener_"; - private static final Logger log = LoggerFactory.getLogger(AbstractConsumerRegistry.class); + protected static final Logger log = LoggerFactory.getLogger(AbstractConsumerRegistry.class); protected ConsumerCache consumerCache; @@ -268,7 +268,7 @@ ProducerInfo producerInfo = loadProducerInfo(id); if (producerInfo == null) { - return Long.MAX_VALUE; + return Long.MIN_VALUE; } else { @@ -280,25 +280,33 @@ { ParameterValidation.throwIllegalArgExceptionIfNull(producerInfo, "ProducerInfo"); - String oldId = update(producerInfo); - - // if we updated and oldId is not null, we need to update the local information - if (oldId != null) + // only save producer info if we have local modifications that postdate last persisted change + if (producerInfo.getLastModified() > getPersistedLastModifiedForProducerInfoWith(producerInfo.getId())) { - WSRPConsumer consumer = createConsumerFrom(producerInfo, true); + String oldId = update(producerInfo); - // update the federating portlet invoker if needed - if (federatingPortletInvoker.isResolved(oldId)) + // if we updated and oldId is not null, we need to update the local information + if (oldId != null) { - federatingPortletInvoker.unregisterInvoker(oldId); + WSRPConsumer consumer = createConsumerFrom(producerInfo, true); + + // update the federating portlet invoker if needed + if (federatingPortletInvoker.isResolved(oldId)) + { + federatingPortletInvoker.unregisterInvoker(oldId); + } + + // update cache + consumerCache.removeConsumer(oldId); + consumerCache.putConsumer(producerInfo.getId(), consumer); } - // update cache - consumerCache.removeConsumer(oldId); - consumerCache.putConsumer(producerInfo.getId(), consumer); + return oldId; } - - return oldId; + else + { + return null; + } } public void start() throws Exception Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/registry/InMemoryConsumerRegistry.java =================================================================== --- components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/registry/InMemoryConsumerRegistry.java 2012-03-12 19:37:32 UTC (rev 8572) +++ components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/registry/InMemoryConsumerRegistry.java 2012-03-12 19:37:41 UTC (rev 8573) @@ -1,6 +1,6 @@ /* * JBoss, a division of Red Hat - * Copyright 2011, Red Hat Middleware, LLC, and individual + * Copyright 2012, Red Hat Middleware, LLC, and individual * contributors as indicated by the @authors tag. See the * copyright.txt in the distribution for a full listing of * individual contributors. @@ -134,7 +134,8 @@ { if (keysToIds.containsValue(id)) { - return consumers.get(id).getProducerInfo(); + final WSRPConsumer consumer = consumers.get(id); + return consumer != null ? consumer.getProducerInfo() : null; } else { Modified: components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/ProducerInfoTestCase.java =================================================================== --- components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/ProducerInfoTestCase.java 2012-03-12 19:37:32 UTC (rev 8572) +++ components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/ProducerInfoTestCase.java 2012-03-12 19:37:41 UTC (rev 8573) @@ -1,6 +1,6 @@ /* * JBoss, a division of Red Hat - * Copyright 2011, Red Hat Middleware, LLC, and individual + * Copyright 2012, Red Hat Middleware, LLC, and individual * contributors as indicated by the @authors tag. See the * copyright.txt in the distribution for a full listing of * individual contributors. @@ -84,6 +84,64 @@ info.setEndpointConfigurationInfo(eci); } + public void testSettersWithoutModificationShouldNotChangeLastModified() + { + final long initial = info.getLastModified(); + + info.setActive(info.isActive()); + assertEquals(initial, info.getLastModified()); + + info.setActiveAndSave(info.isActive()); + assertEquals(initial, info.getLastModified()); + + info.setExpirationCacheSeconds(info.getExpirationCacheSeconds()); + assertEquals(initial, info.getLastModified()); + + info.setId(info.getId()); + assertEquals(initial, info.getLastModified()); + + info.setModifyRegistrationRequired(info.isModifyRegistrationRequired()); + assertEquals(initial, info.getLastModified()); + } + + public void testSettersWithModificationShouldChangeLastModified() throws InterruptedException + { + long initial = info.getLastModified(); + Thread.sleep(10); // to allow for System.currentTimeMillis() to catch up + info.setActive(!info.isActive()); + + initial = info.getLastModified(); + Thread.sleep(10); // to allow for System.currentTimeMillis() to catch up + info.setActiveAndSave(!info.isActive()); + assertTrue(initial != info.getLastModified()); + + initial = info.getLastModified(); + Thread.sleep(10); // to allow for System.currentTimeMillis() to catch up + info.setExpirationCacheSeconds(info.getExpirationCacheSeconds() + 1); + assertTrue(initial != info.getLastModified()); + + initial = info.getLastModified(); + Thread.sleep(10); // to allow for System.currentTimeMillis() to catch up + info.setId(info.getId() + "other"); + assertTrue(initial != info.getLastModified()); + + initial = info.getLastModified(); + Thread.sleep(10); // to allow for System.currentTimeMillis() to catch up + info.setModifyRegistrationRequired(!info.isModifyRegistrationRequired()); + assertTrue(initial != info.getLastModified()); + } + + public void testSetKeyDoesNotChangeLastModified() + { + long initial = info.getLastModified(); + info.setKey(info.getKey()); + assertEquals(initial, info.getLastModified()); + + initial = info.getLastModified(); + info.setKey(info.getKey() + "other"); + assertEquals(initial, info.getLastModified()); + } + public void testSetRegistrationInfo() { RegistrationInfo regInfo = new RegistrationInfo(info);