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

HystrixMetricsPublisherFactory does not respect HystrixPlugins.reset() #455

Closed
mabn opened this issue Jan 7, 2015 · 2 comments · Fixed by #457
Closed

HystrixMetricsPublisherFactory does not respect HystrixPlugins.reset() #455

mabn opened this issue Jan 7, 2015 · 2 comments · Fixed by #457
Milestone

Comments

@mabn
Copy link

mabn commented Jan 7, 2015

The problem is that HystrixMetricsPublisherFactory caches the reference to HystrixMetricsPublisher.

Also it's not possible to really reset it because HystrixMetricsPublisherFactory.INSTANCE must not be null and creating HystrixMetricsPublisherFactory instance calls HystrixPlugins.getMetricsPublisher() which sets metrics publisher to default implementation.

One way to fix it would be to remove HystrixMetricsPublisherFactory.strategy and always call HystrixPlugins.getInstance().getMetricsPublisher() instead.

Test case reproducing this issue:

package com.netflix.hystrix.strategy.metrics;

import static org.junit.Assert.assertNotSame;
import static org.junit.Assert.assertSame;

import org.junit.Test;

import com.netflix.hystrix.HystrixCircuitBreaker;
import com.netflix.hystrix.HystrixCommandGroupKey;
import com.netflix.hystrix.HystrixCommandKey;
import com.netflix.hystrix.HystrixCommandMetrics;
import com.netflix.hystrix.HystrixCommandProperties;
import com.netflix.hystrix.strategy.HystrixPlugins;

public class FactoryResetTest {
    static class CustomPublisher extends HystrixMetricsPublisher{
        private HystrixMetricsPublisherCommand commandToReturn;
        public CustomPublisher(HystrixMetricsPublisherCommand commandToReturn){
            this.commandToReturn = commandToReturn;
        }
        @Override
        public HystrixMetricsPublisherCommand getMetricsPublisherForCommand(HystrixCommandKey commandKey,
                        HystrixCommandGroupKey commandGroupKey, HystrixCommandMetrics metrics, HystrixCircuitBreaker circuitBreaker,
                        HystrixCommandProperties properties) {
            return commandToReturn;
        }
    }

    @Test
    public void testMetricsPublisherReset() {
        // precondition: HystrixMetricsPublisherFactory class is not loaded. Calling HystrixPlugins.reset() here should be good enough to run this with other tests. 

        // set first custom publisher
        HystrixCommandKey key = HystrixCommandKey.Factory.asKey("key");
        HystrixMetricsPublisherCommand firstCommand = new HystrixMetricsPublisherCommandDefault(key, null, null, null, null);
        HystrixPlugins.getInstance().registerMetricsPublisher(new CustomPublisher(firstCommand));

        // ensure that first custom publisher is used 
        HystrixMetricsPublisherCommand cmd = HystrixMetricsPublisherFactory.createOrRetrievePublisherForCommand(key, null, null, null, null);
        assertSame(firstCommand, cmd);

        // reset, then change to second custom publisher
        HystrixPlugins.reset();
        HystrixMetricsPublisherCommand secondCommand = new HystrixMetricsPublisherCommandDefault(key, null, null, null, null);
        HystrixPlugins.getInstance().registerMetricsPublisher(new CustomPublisher(secondCommand));

        // ensure that second custom publisher is used 
        cmd = HystrixMetricsPublisherFactory.createOrRetrievePublisherForCommand(key, null, null, null, null);
        assertNotSame(firstCommand, cmd);
        assertSame(secondCommand, cmd);
    }
}
@mattrjacobs
Copy link
Contributor

Thanks for the report, @mabn, I'll take a look at this later today.

@mattrjacobs
Copy link
Contributor

Thanks for the unit test and suggested fix, @mabn

luan-cestari added a commit to luan-cestari/lightblue-core that referenced this issue Mar 3, 2015
…ly for openjdk 7 in Travis(java 8 worked fine in Travis). Need to update some related dependencies to stable version (not RC) as they included this change Netflix/Hystrix#455
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants