You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by Glen Mazza <gl...@verizon.net> on 2008/01/25 06:47:42 UTC
Re: svn commit: r614645 - in /activemq/trunk/activemq-core/src:
main/java/org/apache/activemq/broker/region/
main/java/org/apache/activemq/broker/region/cursors/
test/java/org/apache/activemq/bugs/
Am Mittwoch, den 23.01.2008, 20:08 +0000 schrieb rajdavies@apache.org:
> 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