Am Mittwoch, den 23.01.2008, 20:08 +0000 schrieb [EMAIL PROTECTED]: > Modified: > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/AbstractPendingMessageCursor.java > URL: > http://svn.apache.org/viewvc/activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/AbstractPendingMessageCursor.java?rev=614645&r1=614644&r2=614645&view=diff > ============================================================================== > --- > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/AbstractPendingMessageCursor.java > (original) > +++ > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/AbstractPendingMessageCursor.java > Wed Jan 23 12:08:27 2008
I think it would be more readable if this class were actually abstract, no? Presently it is instantiable. > } > Modified: > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/StoreDurableSubscriberCursor.java > URL: > http://svn.apache.org/viewvc/activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/StoreDurableSubscriberCursor.java?rev=614645&r1=614644&r2=614645&view=diff > ============================================================================== > --- > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/StoreDurableSubscriberCursor.java > (original) > +++ > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/StoreDurableSubscriberCursor.java > Wed Jan 23 12:08:27 2008 > @@ -74,6 +74,7 @@ > started = true; > super.start(); > for (PendingMessageCursor tsp : storePrefetches) { > + tsp.setMessageAudit(getMessageAudit()); > tsp.start(); > } > } This class has a "private boolean started" value -- I think it would be better if it could rely on the "started/isStarted()" values already in its AbstractPendingMessageCursor base class. Having a "started" value in both classes can be confusing, especially since some of the public/protected methods in the parent abstract class rely on a "started" variable. Also, in each of the subclasses of AbstractPendingMessageCursor, in the method start(), the code sets the value of started=true *before* actually doing any initialization--you may wish to consider moving the started=true assignments to after the initialization, in case something goes wrong with that process. > > Modified: > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/StoreQueueCursor.java > URL: > http://svn.apache.org/viewvc/activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/StoreQueueCursor.java?rev=614645&r1=614644&r2=614645&view=diff > ============================================================================== > --- > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/StoreQueueCursor.java > (original) > +++ > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/StoreQueueCursor.java > Wed Jan 23 12:08:27 2008 > @@ -66,7 +66,9 @@ > nonPersistent.setMaxAuditDepth(getMaxAuditDepth()); > nonPersistent.setMaxProducersToAudit(getMaxProducersToAudit()); > } > + nonPersistent.setMessageAudit(getMessageAudit()); > nonPersistent.start(); > + persistent.setMessageAudit(getMessageAudit()); > persistent.start(); > pendingCount = persistent.size() + nonPersistent.size(); > } > In this class, in the addMessageLast() and addMessageFirst() methods, the code adds to the nonPersistent cursor (and increments the pending count) if and only if the StoreQueueCursor has been started, but in these same methods the code adds to the Persistent cursor regardless of whether the StoreQueueCursor has been started. Is this difference intentional? > Modified: > activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/TopicStorePrefetch.java > URL: > http://svn.apache.org/viewvc/activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region/cursors/TopicStorePrefetch.java?rev=614645&r1=614644&r2=614645&view=diff > ============================================================================== > > public synchronized void addMessageLast(MessageReference node) throws > Exception { > if (node != null) { > - storeMayHaveMoreMessages=true; > + storeHasMessages=true; > node.decrementReferenceCount(); > } > } > > public synchronized void addMessageFirst(MessageReference node) throws > Exception { > if (node != null) { > - storeMayHaveMoreMessages=true; > + storeHasMessages=true; > node.decrementReferenceCount(); > rollback(node.getMessageId()); > } > @@ -240,7 +236,7 @@ > > public void onUsageChanged(Usage usage, int oldPercentUsage,int > newPercentUsage) { > if (oldPercentUsage > newPercentUsage && oldPercentUsage >= 90) { > - storeMayHaveMoreMessages = true; > + storeHasMessages = true; > try { > fillBatch(); > } catch (Exception e) { > >From the above it looks like you might be having problems with inserting spaces for tabs (or vice-versa) within your IDE. Also, in this class, a couple of things look strange with onUsageChanged: public void onUsageChanged(Usage usage, int oldPercentUsage,int newPercentUsage) { if (oldPercentUsage > newPercentUsage && oldPercentUsage >= 90) { storeHasMessages = true; try { fillBatch(); } catch (Exception e) { LOG.error("Failed to fill batch ", e); } } } 1.) I'm not certain here what the code is doing, but shouldn't it be newPercentUsage > oldPercentUsage? 2.) General comment--AFAICT storeHasMessages is very accurately set by addMessageLast() and addMessageFirst(); it would seem to be a potential source of bugs by having this listener also set it. Regards, Glen
