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

Fix leaking local map stats #6971

Merged
merged 1 commit into from Dec 4, 2015

Conversation

Projects
None yet
4 participants
@eminn
Copy link
Collaborator

commented Dec 3, 2015

Below the results from MapMemoryUsageStressTest

Fixes #6972

Heap usage on upstream
screen shot 2015-12-03 at 10 38 48 am

Heap usage with the fixes in this PR
screen shot 2015-12-03 at 10 38 39 am

@eminn eminn added this to the 3.5.5 milestone Dec 3, 2015

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2015

@eminn: could you write a test? I think you could create a map, do some operations, then destroy&re-create the map again -> the stats should be empty?

@eminn

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 3, 2015

run-lab-run

@ahmetmircik

This comment has been minimized.

Copy link
Member

commented Dec 3, 2015

https://hazelcast-l337.ci.cloudbees.com/job/new-lab-fast-pr/326/

Test Result (1 failure / ±0)
com.hazelcast.client.spi.impl.ClientInvocationTest.executionCallback_FailOnShutdown
@ahmetmircik

This comment has been minimized.

Copy link
Member

commented Dec 3, 2015

run-lab-run

@eminn eminn force-pushed the eminn:fix/leaking-map-stats branch from 631b99a to 785f128 Dec 3, 2015

@@ -29,6 +34,25 @@
public class LocalMapStatsTest extends HazelcastTestSupport {

@Test
public void testMapStatsAreNotLeaking() throws Exception {

This comment has been minimized.

Copy link
@jerrinot

jerrinot Dec 3, 2015

Contributor

it's something like this sufficient? it makes less assumptions about internals.

    @Test
    public void givenMapContainsStatistics_whenDestroyedAndCreatedAgain_thenStatisticsAreReset()  {
        String mapName = randomMapName();
        HazelcastInstance instance = createHazelcastInstance();
        IMap<Integer, Integer> map = instance.getMap(mapName);
        map.put(1, 1);

        map.destroy();
        map = instance.getMap(mapName);

        long putOperationCount = map.getLocalMapStats().getPutOperationCount();
        assertEquals(0, putOperationCount);
    }

This comment has been minimized.

Copy link
@eminn

eminn Dec 3, 2015

Author Collaborator

It doesn't imply that there no leakage of statistics objects. It could happen that statistics are reset when map is destroyed and the test will pass even they are leaking.

To me both ways are ugly/hacky.

This comment has been minimized.

Copy link
@eminn

eminn Dec 3, 2015

Author Collaborator

Afaik, when getLocalMapStats() is called, statistics object is re-created so it won't test the actual scenario.

This comment has been minimized.

Copy link
@jerrinot

jerrinot Dec 3, 2015

Contributor

this test as it is is failing on the current master -> it shows the stats. data are retained even when the map is destroyed -> leak.

my approach is not bullet-proof and I understand your concern about a hidden leak. yours is more fragile as it makes more assumptions about implementation, but in theory it can reveal even leaks invisible from user API. It's hard to tell what's better -> 👍 from me. You can make your own judgement what approach is better.

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2015

👍

gurbuzali added a commit that referenced this pull request Dec 4, 2015

@gurbuzali gurbuzali merged commit 4b9cef8 into hazelcast:maintenance-3.x Dec 4, 2015

1 check passed

default Build finished. No test results found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.