Fix WAN events fired after split-brain merges where values don't change [HZ-2620] #24928
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current mechanics of WAN replication mean that `MergeOperation` (for `IMap) and `CacheMergeOperation` (for `ICache`) invocations generate a WAN replication event for all results besides no data present on both sides of the merge. This means that a scenario where the values of the merge are identical, and only expiry metadata is updated, still causes WAN replication to take place. This is not desired behaviour as it may introduce unexpected results on the WAN replication target, since the source cluster has not actually had any data changed, it simply recovered from a split-brain scenario. One example of this would be where a target cluster processes WAN replicated data from a source cluster, then deletes it from its local data set once processed. A split-brain merge on the source cluster would cause data that has already been processed on the target cluster to be re-introduced for unnecessary processing. This commit aims to resolve this situation by altering the returned type of the `RecordStore#merge` and `AbstractCacheRecordStore#merge` methods to provide a more verbose result for the merge operation; this allows us to maintain all of the existing functionality, while also allowing the WAN replication stage in `MergeOperation` and `CacheMergeOperation` to check this more verbose response and avoid sending a WAN Replication event when the response is `VALUES_ARE_EQUAL`. The key to this change is that it only changes WAN replication behaviour in this circumstance, and all existing functionality (such as calling entry listeners) remains. I introduced separate response classes for `IMap` vs `ICache` as their functionality and results offered differ slightly; we could get away with using a shared response class and adjusting the internals, but to allow for easy expansion and possible divergence in functionality between the two data structures in the future, I opted to use separate classes. This solution does break the existing API for the `#merge` function in `RecordStore` and `ICacheRecordStore`, but both classes are within our `impl` packages, and so we make no API guarantees about them.
JamesHazelcast
added
Type: Defect
Team: Core
Source: Internal
(I may have had `quick` profile selected in my verify run)
WIP solution that needs feedback and more thought before final approach is decided upon. Without this addition, we get unnecessary WAN events being replicated for backups where we didn't WAN replicate the primary.
This alternative approach is more performant and maintains backwards compatibility.
vbekiaris
reviewed
Jul 3, 2023
hazelcast/src/main/java/com/hazelcast/map/impl/operation/PutAllBackupOperation.java
Outdated
Show resolved
Hide resolved
hazelcast/src/main/java/com/hazelcast/cache/impl/operation/CachePutAllBackupOperation.java
Outdated
Show resolved
Hide resolved
hazelcast/src/main/java/com/hazelcast/cache/impl/operation/CachePutAllBackupOperation.java
Outdated
Show resolved
Hide resolved
|
|
||
| private Map<Data, CacheRecord> cacheRecords; | ||
| private Map<Data, WanWrappedCacheRecord> cacheRecords; |
There was a problem hiding this comment.
What about declaring cacheRecords as Map<Data, ? extends CacheRecord>?
This way we wouldn't impose record-wrapping to all operations (eg loadAll etc) and their serialized form. The disadvantage is there will be an additional complexity (instanceof check, conditionally run/serialize the isWanReplicated logic), so maybe it's not a great trade-off. wdyt?
There was a problem hiding this comment.
I'd actually like to do what I originally thought about doing - converting the Cache backup operations to use a list of pairs, like Map operations do; that way I don't need to have any wrapper at all, and everything can be dealt with in the same manner as Map operations are.
For PoC purposes and as it wasn't difficult to do, I made the changes in this commit: eccd294
Due to all backup operations being wrapped, we can't use our serde trick of appending to the end here.
Currently, Cache uses a Map to store key/value pairs pending backup operations - changing it to a "pairs" List like IMap operations use allows easier integration for things like "non-WAN replicated keys" while also making ICache/IMap mechanics more familiar.
vbekiaris
approved these changes
Jul 7, 2023
hazelcast/src/main/java/com/hazelcast/map/impl/operation/MergeOperation.java
Outdated
Show resolved
Hide resolved
|
Could you please remind what was the rational of this change and the user complaint if there is any? |
This PR removes WAN replication for split-brain merges where the values are equal, but their metadata may still have changed - in this case we should WAN replicate the event so this metadata can be updated on target clusters as well.
|
The job Click to expand the log file-------------------------- ---------SUMMARY---------- -------------------------- [ERROR] COMPILATION ERROR : -------------------------- [ERROR] reason: class file for aQute.bnd.annotation.Resolution not found -------------------------- ---------ERRORS----------- -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/nativememory/HiDensityNativeMemoryCacheRecordStore.java:[305,4] error: method does not override or implement a method from a supertype -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/nativememory/HiDensityNativeMemoryCacheRecordStore.java:[309,79] error: incompatible types: CacheMergeResponse cannot be converted to HiDensityNativeMemoryCacheRecord -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/nativememory/HotRestartHiDensityNativeMemoryCacheRecordStore.java:[30,7] error: merge(CacheMergeTypes,SplitBrainMergePolicy,Object>,CallerProvenance) in HiDensityNativeMemoryCacheRecordStore cannot implement merge(CacheMergeTypes,SplitBrainMergePolicy,Object>,CallerProvenance) in ICacheRecordStore -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/operation/WanCacheMergeOperation.java:[38,46] error: incompatible types: CacheMergeResponse cannot be converted to CacheRecord -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/operation/CacheMergeOperation.java:[68,52] error: incompatible types: CacheMergeResponse cannot be converted to CacheRecord -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/nativememory/HiDensityNativeMemoryCacheRecordStore.java:[306,23] error: merge(CacheMergeTypes,SplitBrainMergePolicy,Object>,CallerProvenance) in HiDensityNativeMemoryCacheRecordStore cannot implement merge(CacheMergeTypes,SplitBrainMergePolicy,Object>,CallerProvenance) in ICacheRecordStore -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/nativememory/HiDensityNativeMemoryCacheRecordStore.java:[305,4] error: method does not override or implement a method from a supertype -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/nativememory/HiDensityNativeMemoryCacheRecordStore.java:[309,79] error: incompatible types: CacheMergeResponse cannot be converted to HiDensityNativeMemoryCacheRecord -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/nativememory/HotRestartHiDensityNativeMemoryCacheRecordStore.java:[30,7] error: merge(CacheMergeTypes,SplitBrainMergePolicy,Object>,CallerProvenance) in HiDensityNativeMemoryCacheRecordStore cannot implement merge(CacheMergeTypes,SplitBrainMergePolicy,Object>,CallerProvenance) in ICacheRecordStore -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/operation/WanCacheMergeOperation.java:[38,46] error: incompatible types: CacheMergeResponse cannot be converted to CacheRecord -------------------------- [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast-enterprise/hazelcast-enterprise/src/main/java/com/hazelcast/cache/impl/hidensity/operation/CacheMergeOperation.java:[68,52] error: incompatible types: CacheMergeResponse cannot be converted to CacheRecord -------------------------- |
ahmetmircik
approved these changes
Jul 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit alters the returned type of the
RecordStore#mergeandAbstractCacheRecordStore#mergemethods to provide a more verbose result for the merge operation; this allows us to maintain all of the existing functionality, while also allowing the WAN replication stage inMergeOperationandCacheMergeOperationto check this more verbose response and avoid sending a WAN Replication event when the response isVALUES_ARE_EQUAL. The key to this change is that it only changes WAN replication behaviour in this circumstance, and all existing functionality (such as calling entry listeners) remains.I introduced separate response classes for
IMapvsICacheas their functionality and results offered differ slightly; we could get away with using a shared response class and adjusting the internals, but to allow for easy expansion and possible divergence in functionality between the two data structures in the future, I opted to use separate classes.I also introduced the same logic handling for backup replica handling (skip WAN replication for these replications when the primary replica was already skipped).
This solution does break the existing API for the
#mergefunction inRecordStoreandICacheRecordStore, but both classes are within ourimplpackages, and so we make no API guarantees about them.Refer to HZ-2620 for detailed breakdown
Fixes https://hazelcast.atlassian.net/browse/HZ-2620
EE PR: https://github.com/hazelcast/hazelcast-enterprise/pull/6197
Breaking changes (list specific methods/types/messages):
ICacheRecordStore#mergeandRecordStore#merge(and implementing classes) functions have had their return type changed. N.B. all changes are within internalimplpackages.