Skip to content

Revert grpc max message size/large timeout workaround for snapshot shipping between raft nodes#2464

Merged
dperny merged 2 commits intomoby:masterfrom
anshulpundir:snap_reset
Dec 4, 2017
Merged

Revert grpc max message size/large timeout workaround for snapshot shipping between raft nodes#2464
dperny merged 2 commits intomoby:masterfrom
anshulpundir:snap_reset

Conversation

@anshulpundir
Copy link
Copy Markdown
Contributor

Since we can now stream snapshots between raft nodes, we can revert the grpc workarounds.

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
@GordonTheTurtle
Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "snap_reset" git@github.com:anshulpundir/swarmkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354330304
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

…shots"

This reverts commit e3e2821.

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 1, 2017

Codecov Report

Merging #2464 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2464      +/-   ##
==========================================
+ Coverage   61.66%   61.68%   +0.01%     
==========================================
  Files         128      128              
  Lines       21076    21069       -7     
==========================================
- Hits        12997    12996       -1     
+ Misses       6677     6657      -20     
- Partials     1402     1416      +14

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Dec 1, 2017

We should hold off on reverting the max message size for a release so that clusters with different versions interoperate more smoothly.

@anshulpundir
Copy link
Copy Markdown
Contributor Author

I disagree, mainly because keeping this would only be useful in cases which should not be supported e.g. trying to add a new manager with an older version of swarmkit where the leader already supports streaming, or upgrading a worker running an older version to a manager where the leader has already been upgraded.

@dperny

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Dec 1, 2017

SGTM, LGTM, you have my approval.

@stevvooe
Copy link
Copy Markdown
Contributor

stevvooe commented Dec 1, 2017

LGTM

I'm assuming we have some regression testing for this.

@anshulpundir
Copy link
Copy Markdown
Contributor Author

I'm assuming we have some regression testing for this.

I'll make sure we do.

@dperny dperny merged commit 1d02732 into moby:master Dec 4, 2017
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