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

*: remove RAFT CF #4372

Open
wants to merge 37 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@overvenus
Copy link
Contributor

commented Mar 14, 2019

What have you changed? (mandatory)

Move all meta data to raft engine and remove RAFT CF of kv engine. This is a breaking change, it changes data organization:

  • Moves store_ident_key/prepare_boostrap_key from kv.default to raft.default
  • Moves apply_state_key/region_state_key from kv.raft to raft.default
  • Remove snapshot_raft_state_key

Because this PR saves apply index to raft engine, it is possible that the apply index in raft engine is out of sync with the actual apply index in kv engine. Thanks to idempotent, as long as we ensure the apply index in raft engine <= the apply index in kv engine, we can just re-apply raft log to restore correct state of kv engine. With this in mind, this PR also made other changes to:

  • Sync kv engine first then sync raft engine.
  • Snapshot, it takes snapshots in apply worker instead region worker, so we can get synced apply state and kv engine snapshot.

What are the type of the changes? (mandatory)

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How has this PR been tested? (mandatory)

Unit tests and integration tests.

Does this PR affect documentation (docs) update? (mandatory)

No.

Does this PR affect tidb-ansible update? (mandatory)

Yes, it is fine to rolling update, but it does not support downgrade to previous versions.

overvenus added some commits Feb 28, 2019

store: upgrade data from v2 to v3
Signed-off-by: Neil Shen <overvenus@gmail.com>
raftstore/store: change bootstrap procedure
Move STORE_IDENT_KEY, PREPARE_BOOTSTRAP_KEY and the initial
bootstrap region from the RAFT CF on kv engine to the raft
engine.

Signed-off-by: Neil Shen <overvenus@gmail.com>
raftstore: partial change snapshot procedure
This commit deprecates snapshot_raft_state_key and change snapshot
procedures: get all meta data from raft engine.

It is a partial change, because there is a corner case. Data may
be changed between taking snapshots of raft engine and kv engine,
which means the applied_index in raft engine may fall far behind
of the actually applied index in kv engine.

Signed-off-by: Neil Shen <overvenus@gmail.com>
server/debug: update raft and kv engine usage for debugger
Signed-off-by: Neil Shen <overvenus@gmail.com>
raftstore: update raft and kv usage for consisitency check
Signed-off-by: Neil Shen <overvenus@gmail.com>
raftstrore: update raft and kv engine usage for apply
Signed-off-by: Neil Shen <overvenus@gmail.com>
*: remove remaining RAFT cf usage
Signed-off-by: Neil Shen <overvenus@gmail.com>
*: deprecate RAFT cf config
Signed-off-by: Neil Shen <overvenus@gmail.com>
*: add tests for upgrading
Signed-off-by: Neil Shen <overvenus@gmail.com>
*: add tests for bootstrapping
Signed-off-by: Neil Shen <overvenus@gmail.com>
raftstore/store: take snapshots in apply worker
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 14, 2019

/release

Address lint
Signed-off-by: Neil Shen <overvenus@gmail.com>
@sre-bot

This comment was marked as outdated.

@overvenus

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 14, 2019

/run-all-tests

Address lints and skip upgrade if it's a new node
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 14, 2019

/release

@sre-bot

This comment was marked as outdated.

Address lints and update tests
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 18, 2019

/release

@sre-bot

This comment was marked as outdated.

overvenus added some commits Mar 18, 2019

util: change crit to error
Signed-off-by: Neil Shen <overvenus@gmail.com>
Merge branch 'master' into remove/raftcf
Signed-off-by: Neil Shen <overvenus@gmail.com>
Revert "util: change crit to error"
This reverts commit 45c1885.

Signed-off-by: Neil Shen <overvenus@gmail.com>

@overvenus overvenus force-pushed the overvenus:remove/raftcf branch from c883678 to 86559c9 Mar 25, 2019

@overvenus

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

/run-integration-tests

@overvenus

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 25, 2019

/release

Address fmt lints
Signed-off-by: Neil Shen <overvenus@gmail.com>
Merge branch 'master' into remove/raftcf
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

/run-all-tests

@@ -57,11 +57,12 @@ pub fn delete_all_in_range_cf(
end_key: &[u8],
use_delete_range: bool,
) -> Result<()> {
// CF_RAFT has been removed!
assert_ne!(cf, crate::CF_RAFT);

This comment has been minimized.

Copy link
@BusyJay

BusyJay Apr 29, 2019

Contributor

I don't think the check is necessary, L62 will return error. And error is better than panic as client can send any messages.


// Deprecated since v3.x.
#[doc(hidden)]
#[serde(skip_serializing, skip_deserializing)]

This comment has been minimized.

Copy link
@BusyJay

BusyJay Apr 29, 2019

Contributor

I don't think you should skip deserializing. What if they are customized before?

overvenus added some commits Apr 29, 2019

Address comments
Signed-off-by: Neil Shen <overvenus@gmail.com>
Merge branch 'master' into remove/raftcf
Signed-off-by: Neil Shen <overvenus@gmail.com>

@overvenus overvenus force-pushed the overvenus:remove/raftcf branch from 5049cf1 to 7bbe7c4 Apr 29, 2019

Address comments
Signed-off-by: Neil Shen <overvenus@gmail.com>
Show resolved Hide resolved src/raftstore/store/fsm/apply.rs Outdated
Show resolved Hide resolved src/raftstore/store/fsm/apply.rs Outdated
}
self.delegate.write_apply_state(apply_ctx.raft_wb_mut());

This comment has been minimized.

Copy link
@BusyJay

BusyJay Apr 29, 2019

Contributor

What if snapshot is requested before a snapshot is applied? For example, a leader requests a snapshot, but the messages buffered in channel for a long time that it step down and start accepting a snapshot from a new leader. So apply state will be written in both raftstore and apply threads.

raftstore: ensure no pending snapshot requests before applying snapshots
Signed-off-by: Neil Shen <overvenus@gmail.com>

@overvenus overvenus force-pushed the overvenus:remove/raftcf branch from cc2b602 to edc2807 Apr 29, 2019

let mut write_opts = WriteOptions::new();
write_opts.set_sync(self.enable_sync_log && self.sync_log_hint);

This comment has been minimized.

Copy link
@hicqu

hicqu May 6, 2019

Contributor

Please remove self.enable_sync_log.

This comment has been minimized.

Copy link
@overvenus

overvenus May 7, 2019

Author Contributor

I think it's already been removed.

self.kv_wb_last_keys = 0;
}
if self.raft_wb.as_ref().map_or(false, |wb| !wb.is_empty()) {
if self.sync_log_hint {

This comment has been minimized.

Copy link
@hicqu

hicqu May 6, 2019

Contributor

Can we reuse the old write_opts?

This comment has been minimized.

Copy link
@overvenus

overvenus May 7, 2019

Author Contributor

I think so, but I don't think it's necessary, it only contains few booleans.

Show resolved Hide resolved src/raftstore/store/peer.rs Outdated
@hicqu

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

rest LGTM.

if data_size > APPLY_WB_SHRINK_SIZE {
// Control the memory usage for the WriteBatch.
self.wb = Some(WriteBatch::with_capacity(DEFAULT_APPLY_WB_SIZE));
self.kv_wb = Some(WriteBatch::with_capacity(DEFAULT_APPLY_WB_SIZE));

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 May 7, 2019

Member

Seems DEFAULT_APPLY_WB_SIZE can be replaced by DEFAULT_KV_WB_SIZE totally.

This comment has been minimized.

Copy link
@overvenus

overvenus May 7, 2019

Author Contributor

Raft write batch is usually small, fewer size saves memory usage.

// so if there is no sync cmd for a long time, this may cause a
// large raft write batch which causes high write duration.
// To keep raft write batch small we can clear it if there is no
// sync cmd in the current cmd batch.

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 May 7, 2019

Member

I'm curious about that when the sync_log_hint is true, but we lost some apply states for some regions. If some of these regions are unlucky and always lost apply states, what will happen?

This comment has been minimized.

Copy link
@overvenus

overvenus May 7, 2019

Author Contributor

It restarts apply from the latest saved applied index, since all commands are idempotent, it will be consistent with other peers eventually.

// when a peer become leader, it will send an empty entry.
// When a peer become leader, it will send an empty entry.
// In order to enable read index as soon as possible, we should persist
// this log immediately.

This comment has been minimized.

Copy link
@zhangjinpeng1987

overvenus added some commits May 7, 2019

Merge branch 'master' into remove/raftcf
Signed-off-by: Neil Shen <overvenus@gmail.com>
apply: gc raft write batch
Signed-off-by: Neil Shen <overvenus@gmail.com>
fix typo
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

/run-all-tests

overvenus added some commits May 9, 2019

Merge branch 'master' into remove/raftcf
Signed-off-by: Neil Shen <overvenus@gmail.com>
tests: cover update apply state unexpectedly
Signed-off-by: Neil Shen <overvenus@gmail.com>
Merge branch 'master' into remove/raftcf
Signed-off-by: Neil Shen <overvenus@gmail.com>

// Deprecated since v3.x.
#[doc(hidden)]
#[serde(skip_serializing)]

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jun 6, 2019

Contributor

How about adding a #[deprecated] attribute?

&keys::region_meta_prefix(1),
&keys::region_meta_prefix(2)
)
.unwrap());
assert!(is_range_empty(

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jun 11, 2019

Contributor

Why remove this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.