You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Rob Godfrey (JIRA)" <qp...@incubator.apache.org> on 2008/11/07 16:47:40 UTC
[jira] Created: (QPID-1440) [Java Client] - Tidy up tasks from
QPID-1289
[Java Client] - Tidy up tasks from QPID-1289
--------------------------------------------
Key: QPID-1440
URL: https://issues.apache.org/jira/browse/QPID-1440
Project: Qpid
Issue Type: Sub-task
Components: Java Client
Affects Versions: M4
Reporter: Rob Godfrey
Assignee: Aidan Skinner
Priority: Minor
Review points from QPID-1289:
AMQSession.java : _defaultPrefetchHighMark; _defaultPrefetchLowMark; should probably have the word "default" removed from their name
AMQConnection.java: protected int _maxPrefetch; ... protected member varibales are (in general) evil... what is the justification for this over using getters/setters?
ClientProperties.java: + public static final String MAX_PREFETCH_DEFAULT = "5000"; What is the reason for making the already large (1000) default even larger? The JIRA text above says make it 500
XAConnectionImpl.java: + return _delegate.createXASession(_maxPrefetch, _maxPrefetch / 2); Why put this logic in the XAConnectionImpl? Surely we should have a method on the underlying connection which takes no arguments - and it can work out the default prefetch values?
StreamMessageTest.java:
- consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK,
- AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
+ consumerSession.createConsumer(queue, Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), false, false, (String) null, ft);
This seems like a change in the test... what it really seems to suggest is that there should be a "createConsumer" method where you don't have to pass this in... since we clearly do not care about it in this instance, and we are having to repeat code that should be elsewhere (getting the defaults from client properties)
ReturnUnroutableMandatoryMessageTest.java : same comment as for Stream message test
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-1440) [Java Client] - Tidy up tasks from
QPID-1289
Posted by "Martin Ritchie (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1440?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12718108#action_12718108 ]
Martin Ritchie commented on QPID-1440:
--------------------------------------
I think that if we are adjusting the prefetch we should make it 1. That way users have to think about what it means to have a prefetch.
> [Java Client] - Tidy up tasks from QPID-1289
> --------------------------------------------
>
> Key: QPID-1440
> URL: https://issues.apache.org/jira/browse/QPID-1440
> Project: Qpid
> Issue Type: Sub-task
> Components: Java Client
> Affects Versions: M4
> Reporter: Rob Godfrey
> Priority: Minor
>
> Review points from QPID-1289:
> AMQSession.java : _defaultPrefetchHighMark; _defaultPrefetchLowMark; should probably have the word "default" removed from their name
> AMQConnection.java: protected int _maxPrefetch; ... protected member varibales are (in general) evil... what is the justification for this over using getters/setters?
> ClientProperties.java: + public static final String MAX_PREFETCH_DEFAULT = "5000"; What is the reason for making the already large (1000) default even larger? The JIRA text above says make it 500
> XAConnectionImpl.java: + return _delegate.createXASession(_maxPrefetch, _maxPrefetch / 2); Why put this logic in the XAConnectionImpl? Surely we should have a method on the underlying connection which takes no arguments - and it can work out the default prefetch values?
> StreamMessageTest.java:
> - consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK,
> - AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
> + consumerSession.createConsumer(queue, Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), false, false, (String) null, ft);
> This seems like a change in the test... what it really seems to suggest is that there should be a "createConsumer" method where you don't have to pass this in... since we clearly do not care about it in this instance, and we are having to repeat code that should be elsewhere (getting the defaults from client properties)
> ReturnUnroutableMandatoryMessageTest.java : same comment as for Stream message test
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Commented: (QPID-1440) [Java Client] - Tidy up tasks from
QPID-1289
Posted by "Martin Ritchie (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1440?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12763478#action_12763478 ]
Martin Ritchie commented on QPID-1440:
--------------------------------------
There are 6 points classes with change requests:
AMQSession.java : DONE
- Variable Rename :
AMQConnection.java: DONE
- Variable was protected to be reused in XAConnectionImpl.. but we have a getter. Swapped to use getter, made variable private. No Setter is needed
ClientProperties.java: DONE
- Set Prefetch Default to 500
XAConnectionImpl.java: DONE
- Now that code uses getter for _maxPrefetch it doesn't need to be in the XAConnectionImpl added createXASession() that defines the default prefetch for that session.
StreamMessageTest.java: NOT CHANGED
- True we do not care about the prefetch values but we care about setting the FieldTable argument. This feature is used so rarely .. as in only for HeadersExchange, I don't think it is worth expanding the qpid.jms interface and having a new method, and the implied functionality, we must continue to support.
ReturnUnroutableMandatoryMessageTest.java : NOT CHANGE
Same comment as above.
> [Java Client] - Tidy up tasks from QPID-1289
> --------------------------------------------
>
> Key: QPID-1440
> URL: https://issues.apache.org/jira/browse/QPID-1440
> Project: Qpid
> Issue Type: Sub-task
> Components: Java Client
> Affects Versions: M4
> Reporter: Rob Godfrey
> Assignee: Martin Ritchie
> Priority: Minor
>
> Review points from QPID-1289:
> AMQSession.java : _defaultPrefetchHighMark; _defaultPrefetchLowMark; should probably have the word "default" removed from their name
> AMQConnection.java: protected int _maxPrefetch; ... protected member varibales are (in general) evil... what is the justification for this over using getters/setters?
> ClientProperties.java: + public static final String MAX_PREFETCH_DEFAULT = "5000"; What is the reason for making the already large (1000) default even larger? The JIRA text above says make it 500
> XAConnectionImpl.java: + return _delegate.createXASession(_maxPrefetch, _maxPrefetch / 2); Why put this logic in the XAConnectionImpl? Surely we should have a method on the underlying connection which takes no arguments - and it can work out the default prefetch values?
> StreamMessageTest.java:
> - consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK,
> - AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
> + consumerSession.createConsumer(queue, Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), false, false, (String) null, ft);
> This seems like a change in the test... what it really seems to suggest is that there should be a "createConsumer" method where you don't have to pass this in... since we clearly do not care about it in this instance, and we are having to repeat code that should be elsewhere (getting the defaults from client properties)
> ReturnUnroutableMandatoryMessageTest.java : same comment as for Stream message test
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Updated: (QPID-1440) [Java Client] - Tidy up tasks from
QPID-1289
Posted by "Robbie Gemmell (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1440?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Robbie Gemmell updated QPID-1440:
---------------------------------
Affects Version/s: 0.5
Fix Version/s: 0.6
> [Java Client] - Tidy up tasks from QPID-1289
> --------------------------------------------
>
> Key: QPID-1440
> URL: https://issues.apache.org/jira/browse/QPID-1440
> Project: Qpid
> Issue Type: Sub-task
> Components: Java Client
> Affects Versions: M4, 0.5
> Reporter: Rob Godfrey
> Assignee: Rob Godfrey
> Priority: Minor
> Fix For: 0.6
>
>
> Review points from QPID-1289:
> AMQSession.java : _defaultPrefetchHighMark; _defaultPrefetchLowMark; should probably have the word "default" removed from their name
> AMQConnection.java: protected int _maxPrefetch; ... protected member varibales are (in general) evil... what is the justification for this over using getters/setters?
> ClientProperties.java: + public static final String MAX_PREFETCH_DEFAULT = "5000"; What is the reason for making the already large (1000) default even larger? The JIRA text above says make it 500
> XAConnectionImpl.java: + return _delegate.createXASession(_maxPrefetch, _maxPrefetch / 2); Why put this logic in the XAConnectionImpl? Surely we should have a method on the underlying connection which takes no arguments - and it can work out the default prefetch values?
> StreamMessageTest.java:
> - consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK,
> - AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
> + consumerSession.createConsumer(queue, Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), false, false, (String) null, ft);
> This seems like a change in the test... what it really seems to suggest is that there should be a "createConsumer" method where you don't have to pass this in... since we clearly do not care about it in this instance, and we are having to repeat code that should be elsewhere (getting the defaults from client properties)
> ReturnUnroutableMandatoryMessageTest.java : same comment as for Stream message test
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Updated: (QPID-1440) [Java Client] - Tidy up tasks from
QPID-1289
Posted by "Martin Ritchie (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1440?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Martin Ritchie updated QPID-1440:
---------------------------------
Status: Ready To Review (was: In Progress)
> [Java Client] - Tidy up tasks from QPID-1289
> --------------------------------------------
>
> Key: QPID-1440
> URL: https://issues.apache.org/jira/browse/QPID-1440
> Project: Qpid
> Issue Type: Sub-task
> Components: Java Client
> Affects Versions: M4
> Reporter: Rob Godfrey
> Assignee: Martin Ritchie
> Priority: Minor
>
> Review points from QPID-1289:
> AMQSession.java : _defaultPrefetchHighMark; _defaultPrefetchLowMark; should probably have the word "default" removed from their name
> AMQConnection.java: protected int _maxPrefetch; ... protected member varibales are (in general) evil... what is the justification for this over using getters/setters?
> ClientProperties.java: + public static final String MAX_PREFETCH_DEFAULT = "5000"; What is the reason for making the already large (1000) default even larger? The JIRA text above says make it 500
> XAConnectionImpl.java: + return _delegate.createXASession(_maxPrefetch, _maxPrefetch / 2); Why put this logic in the XAConnectionImpl? Surely we should have a method on the underlying connection which takes no arguments - and it can work out the default prefetch values?
> StreamMessageTest.java:
> - consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK,
> - AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
> + consumerSession.createConsumer(queue, Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), false, false, (String) null, ft);
> This seems like a change in the test... what it really seems to suggest is that there should be a "createConsumer" method where you don't have to pass this in... since we clearly do not care about it in this instance, and we are having to repeat code that should be elsewhere (getting the defaults from client properties)
> ReturnUnroutableMandatoryMessageTest.java : same comment as for Stream message test
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Assigned: (QPID-1440) [Java Client] - Tidy up tasks from
QPID-1289
Posted by "Martin Ritchie (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1440?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Martin Ritchie reassigned QPID-1440:
------------------------------------
Assignee: Martin Ritchie
> [Java Client] - Tidy up tasks from QPID-1289
> --------------------------------------------
>
> Key: QPID-1440
> URL: https://issues.apache.org/jira/browse/QPID-1440
> Project: Qpid
> Issue Type: Sub-task
> Components: Java Client
> Affects Versions: M4
> Reporter: Rob Godfrey
> Assignee: Martin Ritchie
> Priority: Minor
>
> Review points from QPID-1289:
> AMQSession.java : _defaultPrefetchHighMark; _defaultPrefetchLowMark; should probably have the word "default" removed from their name
> AMQConnection.java: protected int _maxPrefetch; ... protected member varibales are (in general) evil... what is the justification for this over using getters/setters?
> ClientProperties.java: + public static final String MAX_PREFETCH_DEFAULT = "5000"; What is the reason for making the already large (1000) default even larger? The JIRA text above says make it 500
> XAConnectionImpl.java: + return _delegate.createXASession(_maxPrefetch, _maxPrefetch / 2); Why put this logic in the XAConnectionImpl? Surely we should have a method on the underlying connection which takes no arguments - and it can work out the default prefetch values?
> StreamMessageTest.java:
> - consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK,
> - AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
> + consumerSession.createConsumer(queue, Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), false, false, (String) null, ft);
> This seems like a change in the test... what it really seems to suggest is that there should be a "createConsumer" method where you don't have to pass this in... since we clearly do not care about it in this instance, and we are having to repeat code that should be elsewhere (getting the defaults from client properties)
> ReturnUnroutableMandatoryMessageTest.java : same comment as for Stream message test
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Assigned: (QPID-1440) [Java Client] - Tidy up tasks from
QPID-1289
Posted by "Martin Ritchie (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1440?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Martin Ritchie reassigned QPID-1440:
------------------------------------
Assignee: Rob Godfrey (was: Martin Ritchie)
Can you review my changes and comments please.
> [Java Client] - Tidy up tasks from QPID-1289
> --------------------------------------------
>
> Key: QPID-1440
> URL: https://issues.apache.org/jira/browse/QPID-1440
> Project: Qpid
> Issue Type: Sub-task
> Components: Java Client
> Affects Versions: M4
> Reporter: Rob Godfrey
> Assignee: Rob Godfrey
> Priority: Minor
>
> Review points from QPID-1289:
> AMQSession.java : _defaultPrefetchHighMark; _defaultPrefetchLowMark; should probably have the word "default" removed from their name
> AMQConnection.java: protected int _maxPrefetch; ... protected member varibales are (in general) evil... what is the justification for this over using getters/setters?
> ClientProperties.java: + public static final String MAX_PREFETCH_DEFAULT = "5000"; What is the reason for making the already large (1000) default even larger? The JIRA text above says make it 500
> XAConnectionImpl.java: + return _delegate.createXASession(_maxPrefetch, _maxPrefetch / 2); Why put this logic in the XAConnectionImpl? Surely we should have a method on the underlying connection which takes no arguments - and it can work out the default prefetch values?
> StreamMessageTest.java:
> - consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK,
> - AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
> + consumerSession.createConsumer(queue, Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), Integer.parseInt(ClientProperties.MAX_PREFETCH_DEFAULT), false, false, (String) null, ft);
> This seems like a change in the test... what it really seems to suggest is that there should be a "createConsumer" method where you don't have to pass this in... since we clearly do not care about it in this instance, and we are having to repeat code that should be elsewhere (getting the defaults from client properties)
> ReturnUnroutableMandatoryMessageTest.java : same comment as for Stream message test
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org