ARTEMIS-5107 using wrong value in ReplicationStartSyncMessage ctor #5451
Conversation
| @@ -76,7 +76,7 @@ public static SyncDataType getDataType(byte code) { | |||
|
|
|||
| public ReplicationStartSyncMessage(boolean beforeTwoEighteen) { | |||
| super(REPLICATION_START_FINISH_SYNC); | |||
| this.beforeTwoEighteen = synchronizationIsFinished; | |||
| this.beforeTwoEighteen = beforeTwoEighteen; | |||
There was a problem hiding this comment.
If this.beforeTwoEighteen was always being set false before, due to the synchronizationIsFinished field being false at this point, what are the implications of changing this now such that it might now become true?
This seems to point to a test either not existing or not actually being effective in checking this, or perhaps just not actually needing this/related code if it never actually did anything. It doesnt seem like this change should be happening without some other matching change.
There was a problem hiding this comment.
The main fix for the original issue (i.e. ARTEMIS-3767) was in ReplicationAddMessage and ReplicationAddTXMessage. The only important part of the fix in ReplicationStartSyncMessage is in decodeRest where versions before 2.18.0 will send less data than newer versions. The beforeTwoEighteen variable is not actually needed because sending more data to an older version is not a problem. The older version will simply ignore it. I'm updating the fix to take this into account.
There was a problem hiding this comment.
The original original PR and email discussion had suggested there was an issue with the newer servers sending the additional data to the old broker, but it would certainly seem like thats what it has always continued doing even after the subsequent PR was applied, suggesting that isnt the case (or that in the end noone hit it again).
In general I do think it makes more sense to prevent sending such new data to old brokers when making such changes, but it would appear that has always been the case here, so if it hasnt caused issue then at this stage years later perhaps just removing the gating check is the way to go at this point.
There was a problem hiding this comment.
What are your thoughts on this @clebertsuconic ?
There was a problem hiding this comment.
As I understand it, the problem wasn't so much that the newer server was sending additional data so much as it was sending different data that the older server was choking on.
jbertram
force-pushed
the
ARTEMIS-5107
branch
from
d767718 to
8f0b9cb
Compare
The incorrect value has always been used for the `beforeTwoEighteen` variable. However, this is not actually a problem because the `beforeTwoEighteen` variable is not necessary. It's only job is to prevent newer versions from sending extra data to older versions. However, older version will simply ignore the extra data which means the `beforeTwoEighteen` variable can be removed completely. This same compatibility pattern is used in many places for the Core protocol. The tests added with the original fix successfully reproduced the original problem and those tests still pass even with this variable removed. Also, keep in mind that `decodeRest` is still checking the version so that it doesn't try to read data that doesn't exist from an older version.
jbertram
force-pushed
the
ARTEMIS-5107
branch
from
8f0b9cb to
95602d2
Compare
No description provided.