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

Multiple clusters capability. The api interface has changed because all apis now need an atom identifying the connection. #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cibingeorge
Copy link
Owner

No description provided.

@cibingeorge cibingeorge changed the title v1 of multiple clusters capability. TODO: fix tests Multiple clusters capability. The api interface has changed because all apis now need an atom identifying the connection. Jan 25, 2019
{request_timeout, 20000},
{retry_policy, {default, true}}
]}
{clusters,

Choose a reason for hiding this comment

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

I think will be much better to provide the config as follow:

{clusters, [
    {erlcass_conn, [
        {keyspace, <<"load_test_erlcass">>},
        {contact_points, <<"127.0.0.1">>},
        {contact_points, <<"127.0.0.1">>},
        {latency_aware_routing, true},
        {token_aware_routing, true},
        {number_threads_io, 8},
        {queue_size_io, 128000},
        {core_connections_host, 5},
        {max_connections_host, 5},
        {tcp_nodelay, true},
        {tcp_keepalive, {true, 60}},
        {connect_timeout, 5000},
        {request_timeout, 20000},
        {retry_policy, {default, true}}
    ]}
]} 

Basically:

  • remove name and put it as tuple {name, OptionList}
  • remove cluster_options and add all those on the same level with the keyspace. (Most probably keyspace should be removed when you send the option list to the NIF).


struct enif_cass_cluster

Choose a reason for hiding this comment

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

Why not putting this struct into the .cc file ? In case we need the CassCluster* outside the nif_cass_cluster.cc we can make a function that takes the ERL_NIF_TERM and returns the CassCluster

@silviucpp
Copy link

silviucpp commented Jan 31, 2019

Another thing that I don't like and I think we can change is the fact that for every single query we are doing that get_existing_table_name which does concatenations and then from binary to atom.

I think we can do an improvement here like:

  • Merge the functionality of erlcass_stm_cache and erlcass_stm_sessions into one single ETS table which will have the name of the cluster.

I mean when you add a key just put a tuple of : {stm_sessions, Identifier} or {stm_cache, Identifier}

What do you think ?

Silviu

@cibingeorge
Copy link
Owner Author

  • Merge the functionality of erlcass_stm_cache and erlcass_stm_sessions into one single ETS table which will have the name of the cluster.

@silviucpp I thought about this. If we merge both tables, the behavior is a bit weird when the erlcass genserver crashes. Right now init creates a new cluster and session and creates a prepared statement for every cached statement in erlcass_stm_cache. Since erlcass_stm_sessions is a child of the erlcass, that dies as well. A solution could be to remember the cluster and session in the new ets table and use it if it exists when init-ing erlcass.

The only alternate solution i can think of is this binary to existing atom code.

…ll apis now need an atom identifying the connection.
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.

2 participants