Skip to content

v1.0 pre - #94

Merged
matehat merged 18 commits into
masterfrom
v1.0-pre
May 12, 2016
Merged

v1.0 pre#94
matehat merged 18 commits into
masterfrom
v1.0-pre

Conversation

@matehat

@matehat matehat commented Apr 24, 2016 •
edited
Loading

Copy link
Copy Markdown
Collaborator
This is a work-in-progress PR for version 1.0

Suggested changes:

  1. Make hash mode the default (addresses Doesn't seem to scale... #49), and still support pooler mode when set explicitly
  2. Support multiple configured C* clusters (addresses We may need a way to allow cqerl to support multiple clusters #59), with simpler old-style "single-cluster" mode seamlessly supported
  3. Discuss if we can bake in token-aware policy (addresses Add support for Token Aware policy #48)
Cluster support

The information about the members of a cluster is kept in a protected ets table, readable from the user's process, so no bottleneck is introduced in this mechanism, just one more hop to a ETS table.

%% sys.config

{cqerl, [ {cassandra_clusters, #{
    change_log => [
      { {"123.123.123.123", 9042}, [{keyspace, change_log}]},
      { {"124.124.124.124", 9042}, [{keyspace, change_log}]}
    ]
}]}

> cqerl_cluster:get_any_from_cluster(change_log).
{ok, {<0.78.0>,#Ref<0.0.8.367>}}

> cqerl_cluster:add_clients_to_cluster(messages, [
    { {"123.123.123.123", 9042}, [{keyspace, messages}]},
    { {"124.124.124.124", 9042}, [{keyspace, messages}]}
]).
ok

> cqerl_cluster:get_any_from_cluster(messages).
{ok, {<0.73.0>,#Ref<0.0.8.366>}}
Simple clusters
%% sys.config

{cqerl, [ {cassandra_nodes, [
      { {"123.123.123.123", 9042}, [{keyspace, change_log}]}
]]}

> cqerl_cluster:add_clients([
    { {"124.124.124.124", 9042}, [{keyspace, change_log}]}
]).
ok

> cqerl_cluster:get_any().
{ok, {<0.73.0>,#Ref<0.0.8.366>}}

@matehat

matehat commented Apr 24, 2016

Copy link
Copy Markdown
Collaborator Author

@leiyangyou is the above something that you think addresses the need you expressed in #59?

@matehat

matehat commented Apr 24, 2016

Copy link
Copy Markdown
Collaborator Author

@bernardd what do you think of this approach?

@bernardd

bernardd commented Apr 25, 2016

Copy link
Copy Markdown
Collaborator

Looks mostly fine to me. My main comment would be that the calls cqerl_cluster:get_any() and cqerl_cluster:get_any_from_cluster(change_log) are possibly better exposed through the cqerl_client module (even if they're implemented in cqerl_cluster). At the moment, it's not clear from either the module name or the function that you're actually getting a client handle. Alternatively, maybe they could be grouped as cqerl_cluster:get_any_client/0,1.

bernardd
bernardd reviewed Apr 25, 2016
View reviewed changes
Comment thread src/cqerl_cluster.erl Outdated
end, sets:to_list(NewClients)),
{noreply, undefined}.

handle_info(timeout, _State) ->

@bernardd bernardd Apr 25, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could this stuff not be done in init/1? The only catch I can see is the call to cqerl_hash:get_client/1, but cqerl_hash should already be started by the time this module starts up. But it's very likely I'm missing something :)

@matehat matehat Apr 26, 2016

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It was mainly to not block the supervisor trying to spawn the cluster gen_server process, while possibly multiple groups of clients are being started.

It is probably not necessary, I've just grown accustomed to not do anything too expensive in init/1 😃

@bernardd bernardd Apr 26, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Eh, it's fair enough. Thinking on it more, it's probably harmless at worst and a good idea at best :)

@matehat

matehat commented Apr 26, 2016

Copy link
Copy Markdown
Collaborator Author

About the exposed API, would you see it exposed as cqerl:get_client/0,1?

@bernardd

bernardd commented Apr 26, 2016

Copy link
Copy Markdown
Collaborator

Actually yeah - I said cqerl_client but forgot that we hide even that behind the cqerl interface.

@bernardd

bernardd commented May 3, 2016

Copy link
Copy Markdown
Collaborator

Would it be possible to get the cluster stuff merged in on its own, separate from the other items? I ask because we're just at a point now where it would be really handy to have it :)

@matehat

matehat commented May 3, 2016 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@bernardd I added a new branch called cluster-support, which is just a cherry-pick of cluster-related commits. I've been using it in our project and it works well, but my main hesitation to merge it in master is the lack of a test suite and proper documentation around that new functionality. I didn't find the time yet to do them.

@bernardd

bernardd commented May 4, 2016

Copy link
Copy Markdown
Collaborator

Brilliant, thanks. If I have some time I'll try to add some.

@matehat

matehat commented May 12, 2016

Copy link
Copy Markdown
Collaborator Author

Now has docs and tests, merging!

@matehat
matehat merged commit 7168c59 into master May 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@matehat @bernardd