search

flightstats / hub

fault tolerant, highly available service for data storage and distribution
http://www.flightstats.com
MIT License
103 stars 35 forks source link

Config module #1167

Closed manjula-chikkanagappa closed 5 years ago

lkemmerer commented 5 years ago

@manjula-chikkanagappa What are your thoughts on leveraging Guice and Names.bindProperties to inject configuration properties into our DI framework? It'd get rid of the awkward PropertyLoader.getInstance() method calls... I was also hoping to use Guice Providers to build up Lombok'ed config POJOs so that logic and defaults could be centralized and tested more explicitly. MetricsConfig and MetricsConfigProvider kind of go that route without having done the property guicification. ...I'm not sure how much static hub weirdness you'd have to overcome with that approach. ...guice does provide a crutch to help deal with that, at least...

manjula-chikkanagappa commented 5 years ago

@manjula-chikkanagappa What are your thoughts on leveraging Guice and Names.bindProperties to inject configuration properties into our DI framework? It'd get rid of the awkward PropertyLoader.getInstance() method calls... I was also hoping to use Guice Providers to build up Lombok'ed config POJOs so that logic and defaults could be centralized and tested more explicitly. MetricsConfig and MetricsConfigProvider kind of go that route without having done the property guicification. ...I'm not sure how much static hub weirdness you'd have to overcome with that approach. ...guice does provide a crutch to help deal with that, at least...

Yep I know. As much as I would like to use @Inject, some of the classes are instantiated manually. These thing will have to to be fixed eventually. One step at a time. Since these changes span all over the hub app, I would like limit the number of changes and also its impact on unit and integration tests. Also the approach I have taken to modularize the config properties is much simpler and also reduces the number of classes.

manjula-chikkanagappa commented 5 years ago

Wow, that was a lot of code to change; nice job sticking with it! After looking at the code more, I agree that your way vs the Provider + POJO is generally more elegant.

One thing I noticed and started commenting on before I realized there'd be a lot of the same comments is that there are a large number of places where the variables for injected dependencies should be final. If you want a hand finding and changing them, let me know. I know this required a lot of rote and boring changes and I'm happy to share the work.

Once this is in, I'd like to figure out a way to get rid of the PropertyLoader.getInstance thing. I know it means we'll need to refactor away a lot of static methods, but I think it'll be worth it. I also really want to get rid of setProperty. Having app-wide mutable state like that has only ever brought trouble in my experience.

Wow, that was a lot of code to change; nice job sticking with it! After looking at the code more, I agree that your way vs the Provider + POJO is generally more elegant.

One thing I noticed and started commenting on before I realized there'd be a lot of the same comments is that there are a large number of places where the variables for injected dependencies should be final. If you want a hand finding and changing them, let me know. I know this required a lot of rote and boring changes and I'm happy to share the work.

Once this is in, I'd like to figure out a way to get rid of the PropertyLoader.getInstance thing. I know it means we'll need to refactor away a lot of static methods, but I think it'll be worth it. I also really want to get rid of setProperty. Having app-wide mutable state like that has only ever brought trouble in my experience.

I suggest we make these config related changes in phases. The reduces the scope of testing, simplify the debugging and rollbacks if required. PropertyLoader.getInstance and setproperty was introduced to support existing code. Once we get rid of static classes and remove deprecated HubProvider classes, hopefully we will no longer need these hacks. I have created Jira tasks to accomplish these tasks in phases. Please feel free to add to it. I have addressed some of your comments like renaming classes to plural (properties), making variables final, segregating the properties. Anything else I would take up in a separate task.

lkemmerer commented 5 years ago

For sure. I didn't expect to change any of the PropertyLoader behavior in this PR, but wanted to make sure it gets tracked and done eventually. 👍 I'll take a look at the tasks. Thanks!

lkemmerer commented 5 years ago

I've gone through and resolved the comments that I've made that have either been addressed or have tasks added to fix 'em. I'll add some more subtasks and resolve more comments with links to the tasks shortly.

manjula-chikkanagappa commented 5 years ago

Some of these are repeat comments and some are new but based on code that hasn't changed since your last round of feedback changes. Sorry for both...it's a lot of code and I think I lost track of what I've already commented on. :-/ I think most of it is tiny renames. The only non-trivial changes that I can think of are:

  1. Making sure all the new Properties classes are bound as eager singletons
  2. Checking out the implications of loading the properties instance in HubMain as a static without the app's arguments.
  3. Sorting out how to avoid instantiating a static variable in a constructor in LocalWebhookManager.

LocalWebhookManager should be handled as part of task that will remove the deprecated HubProvider class.

lkemmerer commented 5 years ago

LocalWebhookManager should be handled as part of task that will remove the deprecated HubProvider class.

I think it should be handled here as there's a non-zero chance it's introducing a new bug or at least a changing previous application behavior.

manjula-chikkanagappa commented 5 years ago

LocalWebhookManager should be handled as part of task that will remove the deprecated HubProvider class.

I think it should be handled here as there's a non-zero chance it's introducing a new bug or at least a changing previous application behavior.

Not anymore :) . There is no constructor in LocalWebhookManager!

lkemmerer commented 5 years ago

Oh, cool. I didn't see that you'd pushed a change. Thanks!