search

Netflix / dynomite-manager

A sidecar to manage Dynomite clusters
https://github.com/Netflix/dynomite
Apache License 2.0
91 stars 59 forks source link

Centralize Redis and ARDB RocksDB config #45

Closed akbarahmed closed 8 years ago

akbarahmed commented 8 years ago
  1. Move Redis init scripts (start/stop) to DM config class
  2. Remove old/unused redis init script constants
  3. Rename and move Redis persistence config methods, constants
  4. Centralize method to get the RESP backend type
  5. Centralize ARDB RocksDB conf and init methods in DM configuration
  6. Reformat entire ArdbRocksDbRedisCompatible.java file to use spaces not tabs
ipapapa commented 8 years ago

I do not think it is a good strategy to make DynomiteManagerConfiguration a huge class. What if we want to add more storage engines in the future? This is probably not an approach that can scale. The reason some of these were recently moved to the storage class https://github.com/Netflix/dynomite-manager/pull/39/files was to make the code more readable.

We probably need to find a better way... Potentially have some interfaces around which storage engine it is and look in the proper location? i.e. follow some Java design patterns.

I have started a project to bind dynamically the implementations of the interfaces: https://github.com/Netflix/dynomite-manager/issues/41 but I did not have the chance to finish it off as it requires JustInTimeBindngs. More info:

akbarahmed commented 8 years ago

Agreed. I can cancel this PR.

akbarahmed commented 8 years ago

Submitting new PR. Created a new branch with a clean history for this PR.