dynomite icon indicating copy to clipboard operation
dynomite copied to clipboard

rename dynomite.yml settings

Open akbarahmed opened this issue 9 years ago • 18 comments

The following is a suggestion that I think would make the configuration file slightly more "self documenting".

IMO, there is an opportunity to make the dynomite.yml file clearer by renaming the following options:

  • listen to client_listen: Listen for client (RESP) requests
  • dyn_listen to peer_listen: Listen for Dynomite peer requests

If you think this is okay, I'll submit a PR with the update.

akbarahmed avatar Oct 31 '16 21:10 akbarahmed

Sounds good to me 👍 @timiblossom what do you think? Please make sure that the current unit testing is able to cover the code changes.

ipapapa avatar Oct 31 '16 22:10 ipapapa

One more....

  • server_retry_timeout to rejoin_cluster_wait_period: it's the amount of time an ejected node must wait before rejoining the cluster...and not really a timeout

Or rejoin_wait_period as a shorter alternative.

akbarahmed avatar Nov 01 '16 00:11 akbarahmed

These sound good and I am in favor of rejoin_wait_period instead of rejoin_cluster_wait_period. We want to make sure that nothing breaks from these changes.

ipapapa avatar Nov 09 '16 18:11 ipapapa

@akbarahmed One more thing that came to my mind this morning is that we want to make sure that these changes are backward compatible. From a short discussion I had with @shailesh33 we have agreed that such changes can potentially be backward compatible by supporting the new ones, but if somebody uses the old configurations there can be a warning that X has been deprecated in ver YYY. Let me know if it does not make sense.

ipapapa avatar Nov 16 '16 18:11 ipapapa

@ipapapa Will do.

akbarahmed avatar Nov 16 '16 20:11 akbarahmed

@akbarahmed I am working on pushing some of the command line arguments to the YAML file: https://github.com/Netflix/dynomite/pull/370 (not ready)

YAML file seems to become the industry practice compared to multiple command line arguments. Our changes will probably conflict. Do you have this ready or do you want to wait until I am done to rebase?

ipapapa avatar Nov 20 '16 15:11 ipapapa

I'll rebase after you're done.

akbarahmed avatar Nov 21 '16 00:11 akbarahmed

#370 has been merged... Moving to the next round soon!

ipapapa avatar Nov 30 '16 23:11 ipapapa

@ipapapa @shailesh33 The following are some ideas for updates to the dynomite.yaml options.

Overall, the most important goals should be to make the configuration more understandable for users and to bring standardization to the option naming.

The items below are meant to start the discussion and are not hard suggestions. Please edit, change, update as you see fit.

Suggestions for naming conventions:

  • client_: client
  • peer_: node-to-node (i.e. intra-cluster)
  • node: refers to this dynomite instance
  • server_ or backend_: backend storage server

dynomite.yaml configuration options.

Main bullet point is the current option name. Suggested names are nested bullet points.

  • auto_eject_hosts: should dynomite automatically removed failed nodes from cluster
    • auto_eject_node
  • backlog: max length to which the queue of pending connections may grow
    • max_queue_length
    • max_pending_connection
    • max_connections_backlog
  • client_connections: max number of client connections
    • max_client_connections
  • datacenter: datacenter name
    • No change
  • data_store: backend data storage server. Could also change to text (redis, memcached) vs 0,1
    • backend_server
    • server_type
    • backend_server_type
  • distribution: defines how hashed keys are distributed
    • Removed?
  • dyn_connections: max number of peer connections
    • max_peer_connections
    • peer_connections
  • dyn_listen: listen for Dynomite peer requests (i.e. node to node)
    • peer_listen
  • dyn_port: port used for node-to-node communication
    • peer_port
  • dyn_read_timeout: the node-to-node read timeout (in milliseconds)
    • peer_read_timeout
  • dyn_seed_provider
    • seed_provider
  • dyn_seeds: an array of seed servers
    • seeds
    • seed_servers
  • dyn_write_timeout: the node-to-node write timeout (in milliseconds)
    • peer_write_timeout
  • env: environment
    • No change
  • gos_interval
    • gossip_interval
  • hash: hash algorithm to use when hashing a key
    • No change
    • hash_algorithm
  • hash_tag: a string that defines delimiters that cause the key hash to only use the portion of the key
    • No change
    • hash_delimiter
  • listen: listen for client (RESP) requests
    • client_listen
  • pem_key_file: the pem key
    • No change
  • preconnect: should dynomite preconnect to a backend server
    • backend_preconnect
    • preconnect_to_backend
  • rack: rack name
    • No change
  • read_consistency
    • No change
  • secure_server_option: determines which node-to-node communication will be encrypted
    • cluster_security
    • peer_security
  • server_connections: number of connections to the backend storage server
    • backend_connections
    • max_backend_connections
  • server_failure_limit: the number of consecutive failures after which a node is ejected from the cluster
    • No change
    • node_failure_limit
    • auto_eject_failure_limit
  • server_retry_timeout: the amount of time an ejected node must wait before rejoining the cluster...and not really a timeout
    • No change
    • rejoin_wait_period
    • cluster_rejoin_interval
    • rejoin_interval
  • servers: the backend server (ex. Redis, ARDB) in the form of backend-host:backend-port:weight
    • No change
    • server (singular)
    • backend: I like the idea of using a separate term for the backend server vs. the generic server
    • storage
  • stats_listen: address and port number for the statistics REST endpoint (default: "0.0.0.0:22222")
    • Newly added by Ioannis. No change.
  • stats_interval: statistics aggregation interval in msec (default: 30000 msec)
    • Newly added by Ioannis. No change.
  • timeout: the amount of time in ms that we wait for a connection to a server to for a response from a server
    • server_timeout
    • backend_timeout
  • tokens: node token
    • token
    • node_token
  • write_consistency
    • No change

akbarahmed avatar Dec 03 '16 22:12 akbarahmed

I like the initiative of making the configurations more understandable and categorized. If we decide to change it we will have to be backward compatible. I like the notion of peer_. However I personally like datastore instead of backend since datastore more precisely defines the role of the module/process. So something like: datastore_type, datastore_connections datastore_timeout

shailesh33 avatar Dec 05 '16 06:12 shailesh33

datastore_ is better than the options I suggested, so I'll plus 1 this prefix.

akbarahmed avatar Dec 05 '16 07:12 akbarahmed

This whole auto eject thing needs a thought. We recently introduced backoff mechanism for peer connections. but now with the baggage of these options from previous code base, we have to check if auto eject is even to be done or just exponential backoff pretty much is what we do for all the connections. (They more or less achive the same thing but we need to be uniform here)

  • auto_eject_hosts: should dynomite automatically removed failed nodes from cluster
    • auto_eject_datastore
  • backlog: max length to which the queue of pending connections may grow
    • max_pending_connections
  • client_connections: max number of client connections
    • max_client_connections
  • datacenter: datacenter name
    • No change
  • data_store: backend data storage server. Could also change to text (redis, memcached) vs 0,1
    • datastore_type : keep it uniform
  • distribution: defines how hashed keys are distributed
    • Removed
  • dyn_connections: max number of peer connections
    • Today we use 1 and only 1 connection. There will be corruption if we use more than one connection with today's code.
  • dyn_listen: listen for Dynomite peer requests (i.e. node to node)
    • peer_listen
  • dyn_port: port used for node-to-node communication
    • peer_port
  • dyn_read_timeout: the node-to-node read timeout (in milliseconds)
    • peer_read_timeout
  • dyn_seed_provider
    • seed_provider
  • dyn_seeds: an array of seed servers
    • seeds
  • dyn_write_timeout: the node-to-node write timeout (in milliseconds)
    • peer_write_timeout
  • env: environment
    • No change
  • gos_interval
    • gossip_interval
  • hash: hash algorithm to use when hashing a key
    • hash_algorithm
  • hash_tag: a string that defines delimiters that cause the key hash to only use the portion of the key
    • hash_delimiter
  • listen: listen for client (RESP) requests
    • client_listen
  • pem_key_file: the pem key
    • No change
  • preconnect: should dynomite preconnect to a backend server
    • No Change This applies to every connection not just backend or peer
  • rack: rack name
    • No change
  • read_consistency
    • No change
  • secure_server_option: determines which node-to-node communication will be encrypted
    • peer_security
  • server_connections: number of connections to the backend storage server
    • max_datastore_connections
  • server_failure_limit: the number of consecutive failures after which a node is ejected from the cluster
    • auto_eject_failure_limit
  • server_retry_timeout: the amount of time an ejected node must wait before rejoining the cluster...and not really a timeout
    • rejoin_wait_period
  • servers: the backend server (ex. Redis, ARDB) in the form of backend-host:backend-port:weight
    • datastore
  • stats_listen: address and port number for the statistics REST endpoint (default: "0.0.0.0:22222")
    • Newly added by Ioannis. No change.
  • stats_interval: statistics aggregation interval in msec (default: 30000 msec)
    • Newly added by Ioannis. No change.
  • timeout: the amount of time in ms that we wait for a connection to a server to for a response from a server
    • datastore_timeout
  • tokens: node token
    • No Change
  • write_consistency
    • No change

@ipapapa we left the discussion half way. Have a look again and correct me if you disagree.

shailesh33 avatar Dec 06 '16 00:12 shailesh33

@shailesh33 I agree with the above terms... I think @akbarahmed effort is really timely and it will improve significantly the on boarding of new folks!

ipapapa avatar Dec 06 '16 01:12 ipapapa

I will be adding enable_gossip, mbuf_size and max_msgs as part of the effort moving it from the command line.

ipapapa avatar Dec 08 '16 00:12 ipapapa

#386 has been merged for enable_gossip. Moving on to the other two.

ipapapa avatar Dec 09 '16 03:12 ipapapa

@akbarahmed completed mbuf_size and alloc_msgs_max https://github.com/Netflix/dynomite/pull/387 transfer to YAML, but still allowing backward compatibility. Feel free to do any other YAML changes. I will add the appropriate changes to DM so that it can set them to the YAML.

ipapapa avatar Dec 13 '16 04:12 ipapapa

I suggest we get ride of the server_connections parameter. We can corrupt the data is we have more than one server_connections. There is no binding of client connections to the server connections so a client request can be sprayed on multiple connections leading to out of requests.

shailesh33 avatar Dec 19 '16 16:12 shailesh33

@shailesh33 +1 and it seems you are already on this way :) https://github.com/Netflix/dynomite/pull/393

ipapapa avatar Dec 19 '16 21:12 ipapapa