[jboss-jira] [JBoss JIRA] (JGRP-2433) Smaller changes in Message/BaseMessage

[Bela Ban (Jira)]

    [ https://issues.redhat.com/browse/JGRP-2433?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13946692#comment-13946692 ] 

Bela Ban edited comment on JGRP-2433 at 1/15/20 7:01 AM:
---------------------------------------------------------

* Implemented {{copy()}} in {{BaseMessage}} and also introduced a method {{copyPayload()}} that's overwritten by subclasses
* I guess perf tests should be run once Infinispan has switched to JGroups 5.0 in a temp branch. Preliminary perf numbers for JGroups itself were added to JGRP-2218 some time ago.
* I prefer to continue using a short for {{flags}} and a byte for {{transient_flags}}, and not collapse them into one short, as this would leave me with only 3 additional flags. Once I remove flags (e.g. RSVP...), and possibly also remove the {{dest}} field from {{Message}}, I might reconsider this suggestion... 
* I added {{msg_factory_class}} which requires the fully qualified name of a {{MsgFactory}} impl class; this cannot be set at runtime (as you mentioned, we'd not propagate that to other protocols, which fetch a ref at {{init()}} time).
* Changed ID for message type from byte -> short
* I don't think we'll be able to avoid detecting collisions between different registrations using the same ID until runtime, but since I widened the byte to a short, we have more space and less likelyhood of collisions. Wildfly and Infinispan could for example shared a common document in which they list all message types and thus avoid collisions.
* Added abstract methods {{writePayload()}} and {{readPayload()}} to {{BaseMessage}}. These are the only ones that need to be implemented instead of {{writeTo()}}, {{writeToNoAddresses()}} and {{readFrom()}}
* I don't see why {{size()}} shouldn't be implemented in the base class: subclasses can simply call {{super.size()}} and then add their own size...


was (Author: belaban):
* Implemented {{copy()}} in {{BaseMessage}} and also introduced a method {{copyPayload()}} that's overwritten by subclasses
* I guess perf tests should be run once Infinispan has switched to JGroups 5.0 in a temp branch. Preliminary perf numbers for JGroups itself were added to JGRP-2218 some time ago.
* I prefer to continue using a short for {{flags}} and a byte for {{transient_flags}}, and not collapse them into one short, as this would leave me with only 3 additional flags. Once I remove flags (e.g. RSVP...), and possibly also remove the {{dest}} field from {{Message}}, I might reconsider this suggestion... 
* I added {{msg_factory_class}} which requires the fully qualified name of a {{MsgFactory}} impl class; this cannot be set at runtime (as you mentioned, we'd not propagate that to other protocols, which fetch a ref at {{init()}} time).
* Changed ID for message type from byte -> short
* I don't think we'll be able to avoid detecting collisions between different registrations using the same ID until runtime, but since I widened the byte to a short, we have more space and less likelyhood of collisions. Wildfly and Infinispan could for example shared a common document in which they list all message types and thus avoid collisions.
* Added abstract methods {{writePayload()}} and {{readPayload()}} to {{BaseMessage}}. These are the only ones that need to be implemented instead of {{writeTo()}}, {{writeToNoAddresses()}} and {{readFrom()}}

> Smaller changes in Message/BaseMessage
> --------------------------------------
>
>                 Key: JGRP-2433
>                 URL: https://issues.redhat.com/browse/JGRP-2433
>             Project: JGroups
>          Issue Type: Task
>            Reporter: Bela Ban
>            Assignee: Bela Ban
>            Priority: Minor
>             Fix For: 5.0
>
>
> Based on Pedro's suggestions (copied from \[1\]):
> *Comments/Suggestions*
> I've created a new custom {{Message}} class to replace an ISPN command. It can be found here: https://github.com/pruivo/infinispan/blob/t_jgroups_5/core/src/main/java/org/infinispan/remoting/transport/jgroups/message/BackupAckMessage.java
> * The set/get...Array/Object must have a default implementation in the interface. I doubt that any custom implementation will ever implement those methods. Similar idea for {{hasArray()}}
> * Probably the {{size()}} shouldn't be implemented in the base class. Initially, I forgot to implement it :). A {{headersSize()}} method would be needed.
>  
> * -It would be nice to have an abstract method write/read payload instead of overriding the writeTo/readFrom-
> * -{{copy()}} should be implemented in the base class and be final. Just add an abstract method {{copyPayload()}} that is invoked when the payload needs to be copied. The first parameter for {{copy()}} should have a better name, IMO.-
> * -{{MessageFactory}} is kind of difficult to find. Can we have a method in the {{Channel}}? or {{getMessageFactory()}} or {{registerMessage(type, constructor)}}. Also, I found some protocol creating a new {{MessageFactory}}, they should use the same instance, right? And you allow to set a different  {{MessageFactory}} that isn't propagated everywhere... is there a use case where an user needs to replace the  {{MessageFactory}}? If not, just make it static somewhere-
> * -I'm wondering if there is way to avoid conflict to register a new message type. Imagine that wildfly and ispn creates a set new messages type, the conflict will only be detected at runtime. Also, the type is a byte... there is no much room to add custom messages types- 
> * -Can the {{Flag}} and {{TransientFlag}} use the same short? There are only 2 transient flags so far and they can be the last bits of short. You can use a mask to skip sending them through the network-
> * -no perf-ack numbers-
> \[1\] https://issues.redhat.com/browse/JGRP-2218



--
This message was sent by Atlassian Jira
(v7.13.8#713008)