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

M3DB client asynchronous replication #1859

Merged
merged 64 commits into from
Sep 13, 2019
Merged

Conversation

simonrobb
Copy link
Contributor

@simonrobb simonrobb commented Aug 7, 2019

What this PR does / why we need it:
Adds support for asynchronously replicating client writes to multiple M3DB clusters.

@simonrobb simonrobb requested a review from richardartoul August 7, 2019 18:09
src/dbnode/client/replicated_session.go Show resolved Hide resolved
src/dbnode/client/config.go Outdated Show resolved Hide resolved
src/dbnode/client/multi_cluster_options.go Outdated Show resolved Hide resolved
@robskillington
Copy link
Collaborator

Hey this is cool, adding some comments now.

scripts/development/m3_stack/glide.lock Outdated Show resolved Hide resolved
src/dbnode/client/replicated_session.go Show resolved Hide resolved
src/dbnode/client/replicated_session.go Outdated Show resolved Hide resolved
src/dbnode/client/replicated_session.go Show resolved Hide resolved
src/cluster/client/etcd/config.go Outdated Show resolved Hide resolved
src/dbnode/client/client.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 8, 2019

Codecov Report

Merging #1859 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1859   +/-   ##
=======================================
  Coverage    62.2%    62.2%           
=======================================
  Files        1104     1104           
  Lines      104725   104725           
=======================================
  Hits        65160    65160           
  Misses      35318    35318           
  Partials     4247     4247
Flag Coverage Δ
#aggregator 79.7% <0%> (ø) ⬆️
#cluster 56.3% <0%> (ø) ⬆️
#collector 63.7% <0%> (ø) ⬆️
#dbnode 62.9% <0%> (ø) ⬆️
#m3em 59.6% <0%> (ø) ⬆️
#m3ninx 60.3% <0%> (ø) ⬆️
#m3nsch 51.1% <0%> (ø) ⬆️
#metrics 17.7% <0%> (ø) ⬆️
#msg 74.7% <0%> (ø) ⬆️
#query 67.7% <0%> (ø) ⬆️
#x 70.2% <0%> (ø) ⬆️

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 2cca399...c91cdf7. Read the comment docs.

@simonrobb simonrobb changed the title WIP: M3DB client asynchronous replication M3DB client asynchronous replication Aug 13, 2019
@simonrobb simonrobb force-pushed the srobb/cluster-replication branch 2 times, most recently from 94c1ece to eb7e7ec Compare August 20, 2019 16:29
@simonrobb simonrobb force-pushed the srobb/cluster-replication branch 2 times, most recently from 476cee4 to 90eeb5d Compare September 3, 2019 15:58
@richardartoul richardartoul force-pushed the srobb/cluster-replication branch from 444b2c5 to ca16e6f Compare September 4, 2019 18:47
func (s replicatedSession) Close() error {
err := s.session.Close()
for _, as := range s.asyncSessions {
as.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably want to collect the errors here. (so you know if closing session fails, shouldn't matter too much on shutdown whether an error is returned in the case of an error with a real async session)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if logging is enough, not sure if there's an idiomatic way to handle multiple errors without early abort. Capture and concat better?

@robskillington
Copy link
Collaborator

robskillington commented Sep 10, 2019

Just wondering what the need is for ReplicationOptions, etc. Is there a way it could just be added to the existing Options/AdminOptions? It would help avoid breaking compat on the call to NewClient(...).

@simonrobb simonrobb force-pushed the srobb/cluster-replication branch from f8c4411 to 6e00f60 Compare September 13, 2019 12:59
environment:
- M3DB_HOST_ID=cluster_a_m3db_local_2
volumes:
- "./m3dbnode-cluster-a.yml:/etc/m3dbnode/m3dbnode.yml"
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔥 multi-cluster test ftw

Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM other than minor nits.

@simonrobb simonrobb merged commit 885491a into master Sep 13, 2019
@simonrobb simonrobb deleted the srobb/cluster-replication branch September 13, 2019 17:43
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.

4 participants