alex-rufous commented on a change in pull request #100:
URL: https://github.com/apache/qpid-broker-j/pull/100#discussion_r669981358
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/queue/SortedQueueEntryList.java
##########
@@ -108,28 +108,31 @@ private void insert(final SortedQueueEntry entry)
}
entry.setParent(parent);
- if(entry.compareTo(parent) < 0)
+ if (parent != null)
Review comment:
I do not think that check for null is required here.
The "if" and "while loop" above makes sure that parent is not null.
I also think, that when parent is null, it makes more sense to fail by
throwing an Exception as entry will not be inserted and lost...
I would like to suggest reverting of this particular change...
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/BDBConfiguredObjectRecord.java
##########
@@ -111,7 +111,10 @@ public int hashCode()
@Override
public String toString()
{
- return "BDBConfiguredObjectRecord [id=" + _id + ", type=" + _type + ",
name=" + (_attributes == null ? null : _attributes.get("name")) + ", parents="
+ _parents + "]";
+ return "BDBConfiguredObjectRecord [id=" + String.valueOf(_id)
Review comment:
Though, the purpose of the PR JIRA is to fix null references, as this
toString() is changed , I strongly suggest to replace concatenation with
`String.format()`. The latter works better in toString and makes it easier to
read.
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/transport/MultiVersionProtocolEngine.java
##########
@@ -464,13 +464,16 @@ public void received(QpidByteBuffer msg)
_broker.getEventLogger().message(new PortLogSubject(_port),
PortMessages.UNSUPPORTED_PROTOCOL_HEADER(Functions.str(headerBytes),
-
supportedReplyVersion.toString()));
+
String.valueOf(supportedReplyVersion)));
- try (QpidByteBuffer supportedReplyBuf =
QpidByteBuffer.allocateDirect(supportedReplyBytes.length))
+ if (supportedReplyBytes != null)
Review comment:
The broker is required by the AMQP specification to send a reply with a
latest supported AMQP version.
Thus, if supportedReplyBytes is null, it is a defect.
I would like to suggest adding a check whether supportedReplyBytes is null.
If it is null, we we can iterate via available creators and pickup the highest
version among available and set supportedReplyBytes for the highest available
protocol.
if there is no creator, that would mean that all protocols are disabled....
In that case the broker cannot support messaging... I am not sure what the
broker behaviour should be in this case... It seems that broker is inconsistent
state and possibly would need to bring down...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
