search

elodina / datastax-enterprise-mesos

DataStax Enterprise on Mesos
http://www.elodina.net
15 stars 4 forks source link

Dsem 39 cassandra storage #64

Closed abiletskyi closed 8 years ago

dmitrypekar commented 8 years ago

Good implementation, but have a few comments, that needs to be fixed before merge:

  1. For some reason, can't save 2 nodes. Scenario:
    1. add node id=0, exec select * from ..., get 1 row;
    2. add node id=1, exec select ..., get 1 row;
    3. restart scheduler - broker node list shows 1 node;
  2. Please remove FrameworkState class and use Nodes object directly (probably, removing Storage.save/load args/return object). The primary purpose Nodes object is to hold "framework state". I see no big advantages of having 2 separate object/classes. Removing one, will simplify a code.
  3. C* node installation in vagrant should be:
    • disabled (commented) by default (most of users doesn't need it, except several "special" users);
    • if uncommented, installed only on a single node (master) to save resources;
abiletskyi commented 8 years ago

@dmitrypekar thanks,

  1. will check
  2. I'll change that to comply overall codebase but not exactly agree with you. with that we will have one global component just artificially split into objects. in reality you have Nodes using Storage and Storage modifying Nodes. such codestyle makes adding changes really hard.
  3. b) - thanks, definitely need to be installed on master only a) not sure here. As I understood we are trying to persuade users to use C* storage as the only valid HA storage for dse-mesos. If that's correct assumption (@joestein ?) I'd even make C* storage default: --storage cassandra:9042:127.0.0.1
dmitrypekar commented 8 years ago

Merged. Thank you for you patch.