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

Resolve locks #108

Merged
merged 10 commits into from Aug 28, 2019

Conversation

@sticnarf
Copy link
Contributor

sticnarf commented Aug 25, 2019

When locks are returned, we need to resolve the locks and try again. Locks are resolved in the unit of tuple (transaction, region). We use CleanupRequest to clean the primary lock (if the lock is not committed) or get the commit version. Then, use the commit version to resolve the locks.

In this PR, variants of region errors are removed from the library's error type because the behavior is the same in the library. Also, we shouldn't mix errors from PD with KV region errors. This is fixed too.

sticnarf added 2 commits Aug 25, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf requested review from nrc and AndreMouche and removed request for nrc Aug 25, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:resolve-lock branch from becbc3a to b9929af Aug 26, 2019
CI errors should be fixed and feature gates are removed.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:resolve-lock branch from b9929af to f093c64 Aug 26, 2019
Copy link
Contributor

nrc left a comment

Code looks good, I'm not sure if you're doing the right thing by the protocol, but I'm sure @AndreMouche will know that.

There seems to be quite a bit of logic in the functions in lock.rs, could you add some unit tests please?

fn take_locks(&mut self) -> Vec<kvrpcpb::LockInfo>;
}

macro_rules! dummy_impl_has_locks {

This comment has been minimized.

Copy link
@nrc

nrc Aug 26, 2019

Contributor

Could you use a default implementation of take_locks rather than use this macro?

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 26, 2019

Author Contributor

Seems we can't, unless we use specialization?

This comment has been minimized.

Copy link
@Hoverbear

Hoverbear Aug 26, 2019

Member

Wait, specialization is still not stable? 😭

This comment has been minimized.

Copy link
@nrc

nrc Sep 2, 2019

Contributor

Why doesn't

pub trait HasLocks {
    fn take_locks(&mut self) -> Vec<kvrpcpb::LockInfo> {
        Vec::new()
    }
}

work? It doesn't look like it should need specialisation to me.

(And no, specialisation is a long way from being stable, I think there are some complex outstanding questions around soundness?)

This comment has been minimized.

Copy link
@sticnarf

sticnarf Sep 3, 2019

Author Contributor

Having that default implementation still requires us to impl HasLocks for the response types one by one. (impl<T: KvRequest> doesn't work)

This comment has been minimized.

Copy link
@nrc

nrc Sep 3, 2019

Contributor

But it would mean that you could just have a one line impl for each one, rather than a macro.

This comment has been minimized.

Copy link
@sticnarf

sticnarf Sep 3, 2019

Author Contributor

Oh, I see what you mean. That makes sense.

use std::collections::{HashMap, HashSet};
use std::sync::Arc;

pub async fn resolve_locks(

This comment has been minimized.

Copy link
@nrc

nrc Aug 26, 2019

Contributor

It looks to me like these functions should be methods on PdClient. Is there a reason they're not?

This comment has been minimized.

Copy link
@nrc

nrc Aug 26, 2019

Contributor

Could you document what 'resolve' means

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 26, 2019

Author Contributor

It looks to me like these functions should be methods on PdClient. Is there a reason they're not?

The requests are sent to TiKV instead of PD. So I think it should belong to TiKV requests.

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 26, 2019

Author Contributor

Could you document what 'resolve' means

Ok.

This comment has been minimized.

Copy link
@nrc

nrc Sep 2, 2019

Contributor

ISTM that this is a PD 'method' since we first query the timestamp, then resolve_lock_with_retry is a kv client method, although it is a bit weird that it has to figure out the region from pd (though I see why you'd do it this way). I think that there might be a nice way to factor these functions, at the moment it doesn't quite feel like the abstractions are right.

commit_version: u64,
pd_client: Arc<impl PdClient>,
) -> Result<RegionVerId> {
// TODO: Add backoff and retry limit

This comment has been minimized.

Copy link
@nrc

nrc Aug 26, 2019

Contributor

Seems like a retry limit is very easy to add and prevents us getting stuck in infinite loops. Could you add it please?

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 26, 2019

Author Contributor

I hoped to add this backoff and retry limit to this lock resolving and normal KV requests together. Anyway, it's fine to simply add a retry limit here.

@sticnarf

This comment has been minimized.

Copy link
Contributor Author

sticnarf commented Aug 26, 2019

@nrc The tools we have now (a stateless mock tikv) is not enough to unit test the resolve_locks function. I feel like we defer it a bit.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:resolve-lock branch from 37c6fc6 to 16a2abc Aug 26, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
commit_version: u64,
pd_client: Arc<impl PdClient>,
) -> Result<RegionVerId> {
// TODO: Add backoff

This comment has been minimized.

Copy link
@Hoverbear

Hoverbear Aug 26, 2019

Member

Please make an issue about this when this PR gets merged, so we can not forget about it :)

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 27, 2019

Author Contributor

Of course

src/transaction/requests.rs Outdated Show resolved Hide resolved
src/transaction/requests.rs Outdated Show resolved Hide resolved
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:resolve-lock branch from 1eadf1d to 8d02eea Aug 27, 2019
@sticnarf sticnarf requested review from mapleFU and youjiali1995 Aug 27, 2019
sticnarf added 2 commits Aug 27, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Copy link
Member

Hoverbear left a comment

LGTM as long as issues are filed for the backoff and the resolvelocklite. :) Nice job @sticnarf , as always.

@@ -1 +1 @@
nightly-2019-07-31
nightly-2019-08-25

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Aug 28, 2019

Member

I think we should only do one thing in one PR. Please pay attention to this rule in the future.

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 28, 2019

Author Contributor

Sorry, just find some dependencies cannot compile because they remove the async/await feature gate.

&mut self,
pd_client: Arc<PdC>,
) -> BoxStream<'static, Result<(Self::KeyData, Store<PdC::KvClient>)>> {
let key = mem::replace(&mut self.key, Default::default()).into();

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Aug 28, 2019

Member

It seems repeated with L36-L41, could we avoid it?

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 28, 2019

Author Contributor

Done

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@AndreMouche AndreMouche merged commit 2fb14dd into tikv:master Aug 28, 2019
2 checks passed
2 checks passed
DCO All commits are signed off!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.