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

update nflx plugin to use archaius2 #525

Merged
merged 3 commits into from Feb 23, 2018
Merged

Conversation

brharrington
Copy link
Contributor

Runtime is trying to phase out the usage of archaius1.

Runtime is trying to phase out the usage of archaius1.
@brharrington brharrington added this to the 0.62.0 milestone Feb 22, 2018
@@ -43,13 +39,11 @@

/** Create a new instance. */
@Inject
Plugin(Registry registry) throws IOException {
Plugin(Registry registry, Config config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to abstract out the configuration entirely so we don't force an archaius2 dependency on users. Create an interface like this,

interface PluginConfig {
    boolean isEnabled();
    boolean isLoggingEnabled();
}

Then make archaius2 a compileOnly dependency and do some manual work to fallback from archaius's Config to properties. So, for internal apps we will already have the binding so archaius2 will be used, but for open source apps we won't be forcing an archaius dependency on them.

@Provides
@Singleton
public class PluginConfigProvider implements Provider<PluginConfig> {
    @Inject
    Injector injector;
   
    @Override
    public PluginConfig get() {
        try {
            Config config = injector.getInstance(Config.class);
            return new PluginConfig() {
                @Override
                public boolean isEnabled() {
                    return config.getBoolean(ENABLED_PROP, true);
                }
                @Override
                public boolean isLoggingEnabled() {
                    return config.getBoolean(GC_LOGGING_ENABLED_PROP, true);
                }
            };
        } catch (Exception e) {
            LOGGER.warn("no archaius2 binding found, reading configuration from spectator.properties instead");
            final Properties properties = new Properties();
            try (final InputStream stream = this.getClass().getResourceAsStream("spectator.properties")) {
                properties.load(stream);
            } catch (Exception e) {
                LOGGER.warn("Unable to load spectator.properties.  Using defaults instead");
            }  
            return new PluginConfig() {
                @Override
                public boolean isEnabled() {
                    return Boolean.parseBoolean(properties.getProperty(ENABLED_PROP, "true"));
                 }
                @Override
                public boolean isLoggingEnabled() {
                     return Boolean.parseBoolean(properties.getProperty(GC_LOGGING_ENABLED_PROP, "true"));
                }
            };
        }   
    } 
}

Use optional bindings for PluginConfig so users can customize it to their own configuration mechanism

OptionalBinder.newOptionalBinder(binder(), PluginConfig.class).setDefault().toProvider(PluginConfigProvider.class)

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'd prefer to abstract out the configuration entirely so we don't force an archaius2 dependency on users.

Summary of discussion with @elandau:

Longer term they are looking at also supporting Spring and those applications would not need to use archaius2 for configuration. From my point of view, the whole point of the spectator-nflx-plugin library is to provide the setup needed for the current standard of Guice/Governator/Archaius2. If that is not the goal, then do not install this module. For Spring specifically, we'll likely need to provide a separate setup library at some point.

For now this removes the dependency on archaius1 and we'll revisit later when we look into supporting Spring.

@brharrington brharrington merged commit 2d9330c into Netflix:master Feb 23, 2018
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 this pull request may close these issues.

None yet

2 participants