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

Chunking support #6172

Merged
merged 27 commits into from
Jul 24, 2019
Merged

Chunking support #6172

merged 27 commits into from
Jul 24, 2019

Conversation

jefferai
Copy link
Member

This uses the go-raftchunking library to allow for chunked values.

A config flag (not exposed through CLI for the moment) allows setting the desired max value size. If a submitted value is above raft's suggested threshold it automatically does a chunking apply.

Tests checking for too-big values are now split into too-big and allowed-with-config.

Tests for the validity of the actual middleware are in the middleware's package.

This uses the go-raft-middleware library to allow for chunked commits.
Pushing this up as PoC for the moment, it's missing various things:

* Config flag to enable whether this support is turned on or not (and as
a result for now any code checking sizes is just commented out)
* Tests that check for too-big values are commented out (but should be
reenabled once there's a config flag)
* There are not yet any tests to validate that this _works properly_.
It's been tested using Vault as a client but only against a single node
so far which doesn't really ensure that behavior is correct. This is
mostly because I'm not sure of the best way to test against a cluster,
so with some input from the Consul team this shouldn't be too hard to
test. However, there _are_ tests within the middleware library against
Raft that sends large values concurrently and verifies the FSM after so
it's possible that there isn't all that much needed at the Consul layer.
@jefferai jefferai requested review from banks, mkeeler and a team and removed request for banks and mkeeler July 18, 2019 17:37
@jefferai jefferai added this to the 1.5.3 milestone Jul 22, 2019
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Some minor things. The canRetry thing I'm pretty sure won't work as it is due to our encoding stuff.

agent/consul/rpc.go Outdated Show resolved Hide resolved
agent/txn_endpoint_test.go Outdated Show resolved Hide resolved
agent/txn_endpoint_test.go Outdated Show resolved Hide resolved
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Realised in discussion elsewhere that there is a major issue with snapshotting here that needs to be resolved before we can support this in Consul. It might need significant changes to this library and PR.

@jefferai
Copy link
Member Author

A note: #6206 is a PR into this PR that should implement snapshotting support with a full compliment of tests. As the majority of the code here is related to config I decided to just keep them separate for ease of reviewing the other part.

* Add snapshot support for the chunking FSM

* Add some logic to snapshot save/restore

* Add chunking lifecycle test

* Bump go-raftchunking

* Ensure that Apply never returns nil and use a nil return to know for
sure whether or not a chunked apply was interrupted via a term change.

* Add a requested comment
@banks banks requested a review from a team July 24, 2019 15:14
@banks
Copy link
Member

banks commented Jul 24, 2019

Note #6206 is now merged into this PR so it can be reviewed as one.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

LGTM. I'd like someone else from Consul team to review since I was somewhat involved in the design already and would be good to have my assumptions checked too.

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

LGTM

agent/consul/fsm/snapshot_oss.go Outdated Show resolved Hide resolved
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.

3 participants