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/09/13 16:19:17 UTC

[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #3744: ARTEMIS-3475 avoiding possible recursion on toString and improving ov…

clebertsuconic opened a new pull request #3744:
URL: https://github.com/apache/activemq-artemis/pull/3744


   …eral PacketImpl toString


-- 
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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3744: ARTEMIS-3475 avoiding possible recursion on toString and improving ov…

Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3744:
URL: https://github.com/apache/activemq-artemis/pull/3744#discussion_r711016874



##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/PacketImpl.java
##########
@@ -418,9 +418,14 @@ public boolean isRequiresConfirmations() {
       return true;
    }
 
+   /** extensions of this class are supposed to use getPacketString to provide toString functionality */
    @Override
-   public String toString() {
-      return getParentString() + "]";
+   public final String toString() {
+      return getPacketString() + "]";
+   }
+
+   protected String getPacketString() {
+      return this.getClass().getSimpleName() + "(" + this.getClass().getSimpleName() + ")[type=" + type + ", channelID=" + channelID + ", responseAsync=" + isResponseAsync() + ", requiresResponse=" + isRequiresResponse() + ", correlationID=" + getCorrelationID();

Review comment:
       This prints the class name twice, in format "\<classname\>(\<classname\>)[etc]" which seems unusual. Previously it was doing "PACKET(\<classname\>)[etc]".
   
   I'm guessing since you dropped the PACKET you meant to do that and make it just the class name without braces, I'll tweak it that way that in my PR, and if not it can be changed to the actual intent.
   
   EDIT: I now see it did used to be included a second time inside the [etc attributes] bit. Still seems odd to have it twice at the start, I'll proceed with just removing one for now.

##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/ReplicationResponseMessageV2.java
##########
@@ -62,10 +62,9 @@ public void decodeRest(final ActiveMQBuffer buffer) {
    }
 
    @Override
-   public String toString() {
-      StringBuffer buf = new StringBuffer(getParentString());
+   protected String getPacketString() {
+      StringBuffer buf = new StringBuffer(getPacketString());

Review comment:
       This just introduced the very type of recursion the change was looking to prevent. Will send a PR to fix.




-- 
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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-artemis] brusdev commented on pull request #3744: ARTEMIS-3475 avoiding possible recursion on toString and improving ov…

Posted by GitBox <gi...@apache.org>.
brusdev commented on pull request #3744:
URL: https://github.com/apache/activemq-artemis/pull/3744#issuecomment-918785635


   @clebertsuconic nice work, thanks


-- 
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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-artemis] clebertsuconic merged pull request #3744: ARTEMIS-3475 avoiding possible recursion on toString and improving ov…

Posted by GitBox <gi...@apache.org>.
clebertsuconic merged pull request #3744:
URL: https://github.com/apache/activemq-artemis/pull/3744


   


-- 
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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org