You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/01/11 17:00:54 UTC
[GitHub] [activemq-artemis] franz1981 opened a new pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
franz1981 opened a new pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404
https://issues.apache.org/jira/browse/ARTEMIS-3061
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#issuecomment-758127891
Thanks guys, so let me know if the change I did seems fine, I am just trying to separate the slow/fast paths and save some string comparison and class check
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#issuecomment-758127891
Thanks guys, so let me know if the change I did seems fine, I am just trying to separate the slow/fast path ms save some strong comparison and class check
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#discussion_r555276302
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
##########
@@ -1382,20 +1382,26 @@ public final Object getObjectProperty(String key) {
return AMQPMessageIdHelper.INSTANCE.toCorrelationIdString(properties.getCorrelationId());
}
} else {
- Object value = getApplicationPropertiesMap(false).get(key);
- if (value instanceof UnsignedInteger ||
- value instanceof UnsignedByte ||
- value instanceof UnsignedLong ||
- value instanceof UnsignedShort) {
- return ((Number)value).longValue();
- } else {
- return value;
- }
+ return getApplicationObjectProperty(key);
}
return null;
}
+ private Object getApplicationObjectProperty(String key) {
+ Object value = getApplicationPropertiesMap(false).get(key);
+ if (value instanceof Number) {
Review comment:
What if number is float or double. This then would be invalid
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#issuecomment-758115184
I'd presume so. The code calling it either needs to handle those types, or as its doing convert it to something else if it doesnt.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#issuecomment-758115388
Yeah, what Tim said lol.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] tabish121 commented on pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
tabish121 commented on pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#issuecomment-758115040
> @gemmellr
> Just a qq about this:
>
> ```java
> if (value instanceof UnsignedInteger ||
> value instanceof UnsignedByte ||
> value instanceof UnsignedLong ||
> value instanceof UnsignedShort) {
> return ((Number) value).longValue();
> }
> ```
>
> I've improved things a bit on this PR, but still...is it necessary?
Unless the calling code is able / willing to deal with AMQP primitive types like these unsigned values then yes you still need it. It was added for that reason as these types escaped into the AMQP to Core conversion stuff and it didn't deal with them correctly leading to issues. They are valid types for ApplicationProperties so they need to be accounted for in this context.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#discussion_r555295275
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
##########
@@ -1382,20 +1382,26 @@ public final Object getObjectProperty(String key) {
return AMQPMessageIdHelper.INSTANCE.toCorrelationIdString(properties.getCorrelationId());
}
} else {
- Object value = getApplicationPropertiesMap(false).get(key);
- if (value instanceof UnsignedInteger ||
- value instanceof UnsignedByte ||
- value instanceof UnsignedLong ||
- value instanceof UnsignedShort) {
- return ((Number)value).longValue();
- } else {
- return value;
- }
+ return getApplicationObjectProperty(key);
}
return null;
}
+ private Object getApplicationObjectProperty(String key) {
+ Object value = getApplicationPropertiesMap(false).get(key);
+ if (value instanceof Number) {
Review comment:
hmmm.. yeah.. you're right.. ok!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#issuecomment-758090224
@gemmellr
Just a qq about this:
```java
if (value instanceof UnsignedInteger ||
value instanceof UnsignedByte ||
value instanceof UnsignedLong ||
value instanceof UnsignedShort) {
return ((Number) value).longValue();
}
```
I've improved things a bit on this PR, but still...is it necessary?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#discussion_r555265446
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
##########
@@ -1382,20 +1382,26 @@ public final Object getObjectProperty(String key) {
return AMQPMessageIdHelper.INSTANCE.toCorrelationIdString(properties.getCorrelationId());
}
} else {
- Object value = getApplicationPropertiesMap(false).get(key);
- if (value instanceof UnsignedInteger ||
- value instanceof UnsignedByte ||
- value instanceof UnsignedLong ||
- value instanceof UnsignedShort) {
- return ((Number)value).longValue();
- } else {
- return value;
- }
+ return getApplicationObjectProperty(key);
}
return null;
}
+ private Object getApplicationObjectProperty(String key) {
+ Object value = getApplicationPropertiesMap(false).get(key);
+ if (value instanceof Number) {
Review comment:
Can't we just do the ultimate simplification? I know this would be a semantic change but looking at the usages I only see this being used on filter.
I think it would be safe to change this to:
```java
if (value instanceof Number) {
return ((Number) value).longValue();
}
return value;
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3404: ARTEMIS-3061 AMQPMessage::getDuplicateProperty can save key comparisons and class checks
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3404:
URL: https://github.com/apache/activemq-artemis/pull/3404#discussion_r555276302
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
##########
@@ -1382,20 +1382,26 @@ public final Object getObjectProperty(String key) {
return AMQPMessageIdHelper.INSTANCE.toCorrelationIdString(properties.getCorrelationId());
}
} else {
- Object value = getApplicationPropertiesMap(false).get(key);
- if (value instanceof UnsignedInteger ||
- value instanceof UnsignedByte ||
- value instanceof UnsignedLong ||
- value instanceof UnsignedShort) {
- return ((Number)value).longValue();
- } else {
- return value;
- }
+ return getApplicationObjectProperty(key);
}
return null;
}
+ private Object getApplicationObjectProperty(String key) {
+ Object value = getApplicationPropertiesMap(false).get(key);
+ if (value instanceof Number) {
Review comment:
What if number is float or double
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org