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

follower read with applied index #32

Merged
merged 7 commits into from
Dec 3, 2019
Merged

follower read with applied index #32

merged 7 commits into from
Dec 3, 2019

Conversation

hicqu
Copy link
Contributor

@hicqu hicqu commented Nov 12, 2019

Support follower read with applied index. So that we don't need to get region's applied index again.

Signed-off-by: qupeng qupeng@pingcap.com

Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: qupeng <qupeng@pingcap.com>
@hicqu
Copy link
Contributor Author

hicqu commented Nov 14, 2019

PTAL @sunxiaoguang @lysu thanks!

Signed-off-by: qupeng <qupeng@pingcap.com>
@lilin90 lilin90 requested a review from dcalvin November 15, 2019 08:57
@Hoverbear Hoverbear added the Initial Comment Period This RFC is in the initial comment period, and has quite some time to give input on. label Nov 15, 2019
Signed-off-by: qupeng <qupeng@pingcap.com>
Copy link
Member

@5kbpers 5kbpers left a comment

Choose a reason for hiding this comment

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

LGTM

text/2019-11-12-read-with-index.md Outdated Show resolved Hide resolved
text/2019-11-12-read-with-index.md Outdated Show resolved Hide resolved
This RFC proposes a improvement about getting snapshot on followers, which is
if the read request carries an applied index, the peer can get snapshot locally
wihtout any commucation with its leader, if only it has applied to the given
index. It's useful to reduce latency if the cluster is deployed in multi
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
index. It's useful to reduce latency if the cluster is deployed in multi
index. It's useful to reduce latency if the cluster is deployed across multiple


## Motivation

For clusters deployed in multi datacenters, the system latency could mainly
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For clusters deployed in multi datacenters, the system latency could mainly
For clusters deployed across multiple data centers, system latency mainly

## Motivation

For clusters deployed in multi datacenters, the system latency could mainly
depend on the network RTT between datacenters. For example, suppose PDs are
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
depend on the network RTT between datacenters. For example, suppose PDs are
depends on the network round-trip time (RTT) between data centers. For example, suppose PDs are


The implementation of `GetTsAndReadIndex` will get a timestamp from PD first,
and then call `ReadIndex` of service `Tikv` to get applied indices of all
regions which is carried in `Request`. If any retryable error occurs, the proxy
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
regions which is carried in `Request`. If any retryable error occurs, the proxy
Regions that are carried in `Request`. If any retryable error occurs, the proxy

}
```

A question is how clients can knows `applied_index` for every region? The
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A question is how clients can knows `applied_index` for every region? The
A question is how clients can know `applied_index` for every Region? The

```

A question is how clients can knows `applied_index` for every region? The
answer is RPC `ReadIndex` in service `Tikv`. It's already ready, so we can
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
answer is RPC `ReadIndex` in service `Tikv`. It's already ready, so we can
answer is RPC `ReadIndex` in service `Tikv`. It's already implemented, so we can

### Proxy

As above described, we need to add a proxy service in the major datacenter.
Considering the proxy service is better to be high available, we can put it
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Considering the proxy service is better to be high available, we can put it
Considering that the proxy service is better to be highly available, we can put it

text/2019-11-12-read-with-index.md Outdated Show resolved Hide resolved
@dcalvin
Copy link
Member

dcalvin commented Nov 26, 2019

@hicqu PTAL

Signed-off-by: qupeng <qupeng@pingcap.com>

This RFC proposes an enhancement of getting snapshots on followers. The idea is
if the read request carries an applied index, the peer can get snapshot locally
wihtout any communication with its leader as long as only only it has applied
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
wihtout any communication with its leader as long as only only it has applied
without any communication with its leader as long as only it has applied

This RFC proposes an enhancement of getting snapshots on followers. The idea is
if the read request carries an applied index, the peer can get snapshot locally
wihtout any communication with its leader as long as only only it has applied
to the given index. It's useful to reduce latency if the cluster is deployed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to the given index. It's useful to reduce latency if the cluster is deployed
apply raft logs to the given index. It's useful to reduce latency if the cluster is deployed

}
```

So TiKV can get a snapshot directly after it applys to `applied_index`.
Copy link
Member

Choose a reason for hiding this comment

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

With this implementation, TiKV can apply raft logs to the given index applied_index.

}
```

So TiKV can get a snapshot directly after it applys to `applied_index`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
So TiKV can get a snapshot directly after it applys to `applied_index`.
With this implementation, TiKV can apply raft logs to the given index applied_index.

Copy link
Member

@dcalvin dcalvin left a comment

Choose a reason for hiding this comment

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

Some changes as we talked about earlier. Rest LGTM

Signed-off-by: qupeng <qupeng@pingcap.com>
@hicqu
Copy link
Contributor Author

hicqu commented Dec 3, 2019

PTAL @dcalvin thanks!

Copy link
Member

@dcalvin dcalvin left a comment

Choose a reason for hiding this comment

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

LGTM

@hicqu hicqu merged commit da53883 into master Dec 3, 2019
@hicqu hicqu deleted the read-with-index branch December 3, 2019 06:14
@tisonkun
Copy link
Contributor

@hicqu is there a tracking issue for this feature? I'd like to know how this feature get down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Initial Comment Period This RFC is in the initial comment period, and has quite some time to give input on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants