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

Fixes index creation in wrong map-container problem. #7838

Merged

Conversation

ahmetmircik
Copy link
Member

Bug:
Upon subsequent destroy and creation of imap, there is a possibility that there can be more than one map-container referenced by different record-stores at the same time. Due to that, indexes can be created in an unexpected map-container and this can lead to return less than expected number of results when we query imap.

Fix:
Moved the removal of mapContainer to partition-thread, by making this, i want to be sure that after map#destroy, there is always a new mapContainer instance referenced from a newly created recordstore and that mapContainer instance is different from the removed one.

Test:
Testing this defect is so hard. I was able to reproduce it with adding some sleeps. But still trying to find a simple test also.

@ahmetmircik
Copy link
Member Author

run-lab-run

1 similar comment
@ahmetmircik
Copy link
Member Author

run-lab-run

@ahmetmircik
Copy link
Member Author

Test Result (2 failures / -2)
com.hazelcast.client.heartbeat.ClientHeartbeatTest.testHeartbeatStoppedEvent
com.hazelcast.client.heartbeat.ClientHeartbeatTest.com.hazelcast.client.heartbeat.ClientHeartbeatTest

@ahmetmircik
Copy link
Member Author

run-lab-run

@sancar
Copy link
Contributor

sancar commented Mar 25, 2016

@ahmetmircik These client test failures will be fixed by #7836

@ahmetmircik ahmetmircik force-pushed the fix/3.6.3/mapContainerRefs branch 6 times, most recently from 1463dec to 21fe4ea Compare March 29, 2016 22:54
@@ -284,6 +286,10 @@ public boolean isInvalidationEnabled() {
public InterceptorRegistry getInterceptorRegistry() {
return interceptorRegistry;
}

public boolean canBeRemoved() {
Copy link
Contributor

Choose a reason for hiding this comment

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

a very small thing: something like markForRemoval() could be better. as a developer I do not expect a method starting with can- to have a side-effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, completely removed that method, relying on mapContainers#remove(mapName, mapContainer) should be sufficient.

@ahmetmircik ahmetmircik changed the title [DONT MERGE] Fixes index creation in wrong map-container problem. Upon subsequent … [DONT MERGE] Fixes index creation in wrong map-container problem. Mar 30, 2016
…destroy and creation of imap, there is a possibility that there can be more than one map-container referenced by record-stores at the same time. Due to that indexes can be created in an unexpected map-container and this can lead to return less than expected number of results after querying imap
@ahmetmircik ahmetmircik changed the title [DONT MERGE] Fixes index creation in wrong map-container problem. Fixes index creation in wrong map-container problem. Mar 30, 2016
final PartitionContainer[] containers = partitionContainers;
final Semaphore semaphore = new Semaphore(0);
public void destroyMap(String mapName) {
MapContainer mapContainer = mapContainers.get(mapName);
Copy link
Contributor

Choose a reason for hiding this comment

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

which threads can call this method?
edit: I'll answer this myself. both partition threads & user thread can call this.

Copy link
Member Author

Choose a reason for hiding this comment

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

user + generic operation

@ahmetmircik
Copy link
Member Author

run-lab-run

@ahmetmircik
Copy link
Member Author

verify

@jerrinot
Copy link
Contributor

👍

1 similar comment
@eminn
Copy link
Contributor

eminn commented Mar 30, 2016

👍

This pull request was closed.
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 Team: Core Type: Defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants