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

Cassandra refactor #1847

Merged
merged 3 commits into from May 21, 2018
Merged

Cassandra refactor #1847

merged 3 commits into from May 21, 2018

Conversation

rslota
Copy link
Contributor

@rslota rslota commented May 14, 2018

This PR introduces new implementation of mongoose_cassandra. There are no changes to the API, so the code that uses Cassandra remains unchanged.
The new implementation of Cassandra layer moves all request processing to a worker pool (previously only asynchronous requests were handled this way). This reduces code duplication and improves error handling while also fixes some race conditions that were present in previous implementation.

This PR also adds mongoose_cassandra_SUITE that has some simple "cassandra-only" tests for mongoose_cassandra API. This test suite requires a TCP proxy to be active on top of Cassandra (to enable to connection manipulations like connection drop or connection latency).

@codecov
Copy link

codecov bot commented May 14, 2018

Codecov Report

Merging #1847 into master will increase coverage by 0.09%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1847      +/-   ##
=========================================
+ Coverage    74.6%   74.7%   +0.09%     
=========================================
  Files         298     298              
  Lines       27957   28096     +139     
=========================================
+ Hits        20858   20988     +130     
- Misses       7099    7108       +9
Impacted Files Coverage Δ
src/cassandra/mongoose_cassandra_sup.erl 96.29% <100%> (ø)
src/cassandra/mongoose_cassandra.erl 86.36% <100%> (ø)
src/cassandra/mongoose_cassandra_worker.erl 70.73% <70.73%> (ø)
..._distrib/mod_global_distrib_outgoing_conns_sup.erl 73.91% <0%> (-8.7%) ⬇️
src/mongoose_subhosts.erl 86.66% <0%> (-6.67%) ⬇️
src/mongoose_client_api_sse.erl 75% <0%> (-6.25%) ⬇️
src/mod_push_service_mongoosepush.erl 82.85% <0%> (-5.72%) ⬇️
src/mod_muc_light_utils.erl 85.26% <0%> (-3.16%) ⬇️
src/mod_muc_log.erl 77.69% <0%> (-0.26%) ⬇️
src/ejabberd_c2s.erl 84.44% <0%> (-0.22%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af131fd...ed49eb7. Read the comment docs.

@rslota rslota force-pushed the cassandra_refactor branch 6 times, most recently from 40d2706 to 47eb5fe Compare May 15, 2018 13:38
@rslota rslota changed the title [WIP] Cassandra refactor Cassandra refactor May 16, 2018
@rslota rslota requested a review from fenek May 16, 2018 08:15
@kzemek kzemek requested review from kzemek and removed request for fenek May 16, 2018 12:48
Copy link
Contributor

@kzemek kzemek left a comment

Choose a reason for hiding this comment

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

I like the refactor in general, although it brings some questionable indentation changes (not because we care very much, but because some changes consist mostly of whitespace diff from the fine Emacs indentation style).

One point that I'd like to see addressed though - have you considered using https://github.com/inaka/worker_pool instead of extending the custom pooling solution?

@rslota
Copy link
Contributor Author

rslota commented May 16, 2018

@kzemek Actually, no, I haven't considered using worker_pool. This is part of the old implementation that I haven't changed since it was simple irrelevant to the changes in the general worker logic that were the main focus of this refactor.

I'll reformat the code with emacs, I have no idea when and how the code got reformatted. Also, I'll look into the viability of using worker_pool. If it's gonna simplify the code, I'll go for it.

@rslota
Copy link
Contributor Author

rslota commented May 17, 2018

@kzemek I have reformatted the code with emacs, but introducing worker_pool is not the best idea right now. The changes are not as simple as removing the custom supervisor since worker_pool doesn't allow for nested calls (i.e. worker selects another worker) - such calls may end up in deadlock.
I still think that using worker_pool will be cleaner, but it requires non-trivial changes to the worker's logic, therefore I would prefer to do it as a separate PR.

@rslota rslota force-pushed the cassandra_refactor branch 2 times, most recently from bf1197e to 22c4e81 Compare May 18, 2018 09:02
@kzemek kzemek merged commit 3f24be0 into master May 21, 2018
@kzemek kzemek deleted the cassandra_refactor branch May 21, 2018 09:22
@erszcz erszcz mentioned this pull request May 21, 2018
@fenek fenek added this to the 3.0.0 milestone May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants