Implemented split-brain healing for ISet and IList #11677
Conversation
Donnerbart
changed the title
Donnerbart
changed the title
| String name = container.getName(); | ||
| int batchSize = container.getConfig().getMergePolicyConfig().getBatchSize(); | ||
| SplitBrainMergePolicy mergePolicy = getMergePolicy(container); | ||
| Data partitionAwareName = nodeEngine.getSerializationService().toData(name, StringPartitioningStrategy.INSTANCE); |
There was a problem hiding this comment.
Isn't it better to group them by partition (during prepareMergeRunnable() thus you avoid computing (serialization cost) the partition twice, there & here.
There was a problem hiding this comment.
Good one, fixed this in the other PR for IQueue as well.
| private String quorumName; | ||
| private MergePolicyConfig mergePolicyConfig = new MergePolicyConfig(); |
There was a problem hiding this comment.
Is the new mergePolicy config property covered by the client protocol for dynamically adding config? Is this in a different PR?
(see: AddListConfigMessageTask, AddSetConfigMessageTask, ClientDynamicClusterConfig#addSetConfig, ClientDynamicClusterConfig#addListConfig)
There was a problem hiding this comment.
I have no idea how this works (both, never did anything with the Client Protocol and that part of the dynamic config is also new to me). But we have a similar open issue for other configs (#12126) and Thomas mentioned in his PR, that we anyway need to merge this first, before the Client Protocol can be modified. I would say we'll merge all server-side implementations and then add the merge policies to the Client Protocol in a single PR. WDYT?
There was a problem hiding this comment.
Basically we need to allow adding new configuration from the client when the cluster is running. To allow specifying the merge policy, the client protocol needs to be expanded, e.g. https://github.com/mmedenjak/hazelcast-client-protocol/blob/master/hazelcast/src/main/java/com/hazelcast/client/impl/protocol/template/DynamicConfigTemplate.java#L118-L135
There was a problem hiding this comment.
Merging this PR and addressing it later in a separate PR is fine by me, BTW.
| mergePolicy.setSerializationService(nodeEngine.getSerializationService()); | ||
|
|
||
| // try to find an existing item with the same value | ||
| CollectionItem existingItem = null; |
There was a problem hiding this comment.
I know it's not a clear cut decision but I'm thinking if we should find the existing entry by value or by ID. E.g. how should we behave if we are merging these two lists: [1,1,1,2,3,1] and [2,1,1,3,2,1]. Right now all of the 1s in the list from the smaller cluster will be merged with the same entry in the bigger cluster. Maybe switching to merging entries by ID would produce better results. The same comment was on ringbuffer PR AFAIK.
There was a problem hiding this comment.
The problem with the ID is, that it's completely internal and unrelated to the value. So if we add the same values to a Set in both sub-clusters, but the operations will be handled in a different order, the sets will contain the same items, but with different internal IDs. I don't think merging by those are a good idea. We should merge by what the user knows about the data structure (the contents, not the implementation details).
There was a problem hiding this comment.
Yes, there are actually good points on both approaches. The downside to merging entries with the same ID is, as you say, that the items may just be ordered differently.
The downside to merging entries that are equal are:
- in the degenerative case when the collection from the smaller cluster contains the same entry multiple times, it will be merged with the same entry in the bigger cluster. For instance, if the list in the bigger cluster is
[1,2,3,4,5]and the list from the smaller cluster is[1,1,1,1], all of entries from the smaller cluster will be merged with the same entry in the bigger cluster - there is also the unusual case when the merge policy returns a different value which is then equal to some value further down in the merging collection. For instance, let's say there's a silly policy that returns the
value+1and you're merging[1,2,3,4,5](bigger cluster) with[1,2,3,4,5](smaller cluster, merging list). First the two1s are merged and produce[2,2,3,4,5](bigger cluster) and[2,3,4,5](smaller cluster, merging collection). Now the second entry is again equal to the already merged first entry in the bigger cluster and we get[3,2,3,4,5](bigger cluster) and[3,4,5](smaller cluster, merging collection). Finally, after the merging collection is exhausted we end up with[6,2,3,4,5].
Both approaches have their downsides and it boils down to what one would expect.
I'm thinking that we could merge this and open an issue to consider which approach is better and if needed fix it later. Is that ok?
There was a problem hiding this comment.
In your first example nothing would actually be changed, since the values are not different after the merging. I always check (newValue != null && !newValue.equals(oldValue)), so if the merge policy returns the same value, we just leave everything as it is. Of course it takes some time to process everything, but nothing bad happens.
But yes, I would merge this approach (since it works in its way) and see if there are any flaws later. This applies to any keyless data structure, so we eventually have to solve it in multiple data structures anyway.
| @@ -233,10 +233,12 @@ | |||
|
|
|||
| <list name="default"> | |||
| <backup-count>1</backup-count> | |||
| <merge-policy batch-size="100">PutIfAbsentMergePolicy</merge-policy> | |||
There was a problem hiding this comment.
Can you add it to hazelcast-fullconfig.xml as well?
Depends on #11788
Depends on #12061