Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed IMap.putAll() on bouncing members with PutAllPartitionAwareOperationFactory #8310

Merged
merged 1 commit into from
Jul 4, 2016
Merged

Fixed IMap.putAll() on bouncing members with PutAllPartitionAwareOperationFactory #8310

merged 1 commit into from
Jul 4, 2016

Conversation

Donnerbart
Copy link
Contributor

@Donnerbart Donnerbart commented Jun 3, 2016

Replaced PutAllPerMemberOperation with PutAllPartitionAwareOperationFactory to fix failure in IMap.putAll() on bouncing members.

@Donnerbart Donnerbart self-assigned this Jun 3, 2016
@Donnerbart
Copy link
Contributor Author

Either I didn't understand how to use the PartitionAwareOperationFactory correctly or it just doesn't match the needs of the putAll() method. With this approach I constantly get exceptions like this during migration:

java.lang.IllegalThreadStateException: Thread[hz._hzInstance_2_dev.partition-operation.thread-5,5,_hzInstance_2_dev] cannot make remote call: com.hazelcast.map.impl.operation.PutAllOperation{serviceName='hz:impl:mapService', identityHash=2091219611, partitionId=11, replicaIndex=0, callId=0, invocationTime=-1 (1970-01-01 00:59:59.999), waitTimeout=-1, callTimeout=60000, name=1bcff209-d870-4cf7-8b8b-518d95841f64}

I think the cause it that I still create a PutAllPerMemberOperation which contains data for several partitions. If nodes join or leave those operations may contain data for remote partitions. The PartitionAwareOperationFactory doesn't seem to handle this correctly, since there is no code to re-create a new operation with the correct partitions.

So I think we have to stick with the solution of #8275, even though it creates more code (which I think is needed).

@ahmetmircik
Copy link
Member

@Donnerbart Created a PR based on your commits here #8443 and it seems something like this can be a fix b7a317c

@Donnerbart
Copy link
Contributor Author

Donnerbart commented Jun 22, 2016

Yeah, it seems to work \o/ I also updated this PR with your fix. Thanks a lot for having a look.

@Donnerbart Donnerbart changed the title [DONT MERGE] Fixed IMap.putAll() on bouncing members with PutAllPartitionAwareOperationFactory Fixed IMap.putAll() on bouncing members with PutAllPartitionAwareOperationFactory Jun 22, 2016
@Donnerbart
Copy link
Contributor Author

I will do a comparison with a profiler between master, the other fix approach #8275 and this PR tomorrow. Just to be sure we have no performance degradation or other regression with one of the fixes. If both are equally good, I'll prefer this solution to reduce code duplication. I'll have a look at the EE side then.

@Donnerbart
Copy link
Contributor Author

Hmm, I'm a bit baffled with the benchmark results. This approach with the PartitionAwareOperationFactory should in theory be slower, since it doesn't offer async methods for the putAllInternal() method. So each time an operation is sent to another member, it blocks. So the performance should be especially bad when batching is used (since this increases the number of remote operations).

image

…actory to fix failure in IMap.putAll() on bouncing members.
@devOpsHazelcast
Copy link
Collaborator

Test PASSed.

* @throws Exception
*/
Map<Integer, Object> invokeOnPartitions(String serviceName, OperationFactory operationFactory,
int[] partitions) throws Exception;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have a similar method in OperationService here. Cant we use that instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found no good way to convert an int[] to a Collection<Integer> and I didn't want to introduce litter again. Maybe we can get rid of the other method eventually, since the array should be more litter friendly.

@ahmetmircik ahmetmircik added this to the 3.7 milestone Jun 28, 2016
@ahmetmircik
Copy link
Member

👍

1 similar comment
@gurbuzali
Copy link
Contributor

👍

@Donnerbart Donnerbart merged commit ae72702 into hazelcast:master Jul 4, 2016
@Donnerbart Donnerbart deleted the putAllWithPartitionAwareOperationFactory branch July 4, 2016 12:56
@mmedenjak mmedenjak added the Source: Internal PR or issue was opened by an employee label Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Source: Internal PR or issue was opened by an employee
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants