store: cache region heartbeat #2701

overvenus · 2018-01-19T07:23:57Z

Caching region heartbeats if the current heartbeat is same as the previous one, it should reduce a large number of unnecessary heartbeats.

disksing · 2018-01-21T23:49:44Z

tests/raftstore_cases/test_region_heartbeat.rs

+    // After ttl, there must be exact one heartbeat.
+    sleep(Duration::from_millis(heartbeat_interval_ms * heartbeat_ttl));
+    assert_eq!(pd_client.get_region_heartbeat_count(), count + 1);
+}


I think we need a test that heartbeat is sent when ttl is not expired and heartbeat is updated.

disksing · 2018-01-21T23:53:51Z

Can we make default region heartbeat interval shorter? (to make updating regions update faster)

zhangjinpeng87 · 2018-01-23T06:26:19Z

Also need to handle read statistics at https://github.com/pingcap/tikv/blob/master/src/pd/pd.rs#L511.

overvenus · 2018-02-27T09:46:29Z

A friendly ping.

In tests, the mock pd-server(TestPdClient::Cluster) does not push region heartbeat responses. This PR try to simulate pushing by polling responses in the background. It's part of region heartbeat cache #2701, for fixing tests.

huachaohuang · 2018-03-23T12:03:03Z

src/pd/pd.rs

+        let cache = self.heartbeat_cache
+            .entry(region_id)
+            .or_insert_with(RegionHeartbeatCache::default);
+        info!("debug: before merge {:?}\n{:?}", cache, hb_task);


Remove these debug logs.

huachaohuang · 2018-03-23T12:04:25Z

src/pd/pd.rs

        self.store_stat
            .region_bytes_written
-            .observe(region_stat.written_bytes as f64);
+            .observe(cache.written_bytes_delta as f64);


Does it mean that the previous statistics are wrong?

I think we are both right, the written_bytes is a delta. See line 555 https://github.com/pingcap/tikv/pull/2701/files#diff-a2b393fbe5f436bb88e05e100af8c1b4L555

huachaohuang · 2018-03-23T12:04:57Z

src/pd/pd.rs

        self.store_stat
            .region_keys_read
-            .observe(region_stat.read_keys as f64);
+            .observe(cache.read_keys_delta as f64);

        // Now we use put region protocol for heartbeat.


Wha is "put region protocol"?

overvenus · 2018-03-27T07:32:42Z

PTAL

nolouch · 2018-03-27T08:13:41Z

src/pd/pd.rs

+    fn handle_reconnect(&mut self) {
+        if self.is_handle_reconnect_scheduled {
+            if self.invalidate_cache.swap(false, Ordering::Relaxed) {
+                info!("invalidate region heartbeat cache");


what situation will the runner be re-run?

And we need to reset is_handle_reconnect_scheduled here.

When the client connects to a new PD leader.

hicqu · 2018-03-27T08:35:34Z

src/pd/pd.rs

+                    self.last_written_bytes = total_written_bytes;
+                    self.valid = false;
+                }
+                // TODO: what if last_written_bytes > total_written_bytes?


Why could it happend?

…t-cache

overvenus · 2018-04-02T09:30:36Z

A friendly ping.

nolouch

LGTM.

overvenus · 2018-04-09T04:04:09Z

A friendly ping.

huachaohuang · 2018-04-13T03:15:51Z

src/pd/pd.rs

@@ -486,20 +591,36 @@ impl<T: PdClient> Runner<T> {
        self.is_hb_receiver_scheduled = true;
    }

+    fn handle_reconnect(&mut self) {


This function does two things:

If is_handle_reconnect_scheduled is false, it means that the reconnect handler in pd client hasn't been initialized, we should set it.

If is_handle_reconnect_scheduled is true, it means that we have initialized the reconnect handler, then we should check if a reconnect has happened, and invalidate the cache if it did.

Am I right?

Yes, I prefer that keep things together.

But it is very confusing, it takes me a while to figure this.
I think you can just initialize the reconnect handler somewhere, and just handle reconnect here.

I think we need comments here.

hicqu · 2018-04-19T05:26:32Z

src/pd/pd.rs

        self.store_stat
            .region_bytes_written
-            .observe(region_stat.written_bytes as f64);
+            .observe(cache.written_bytes_delta as f64);


What if written_bytes_delta is not changed between multi handle_heartbeat?

hicqu · 2018-04-19T05:57:07Z

src/raftstore/store/peer.rs

@@ -957,8 +957,8 @@ impl Peer {
        }
        self.mut_store().apply_state = res.apply_state.clone();
        self.mut_store().applied_index_term = res.applied_index_term;
-        self.peer_stat.written_keys += res.metrics.written_keys;
-        self.peer_stat.written_bytes += res.metrics.written_bytes;
+        self.peer_stat.total_written_keys += res.metrics.written_keys;


Do we need to update peer_stat for followers and learners? And, I think we also need to clear peer_stat in on_role_changed is we do so.

If we change peer_stat's behavior, it may incompatible with PD and it becomes a breaking change.

src/pd/pd.rs

hicqu · 2018-04-19T06:06:43Z

rest LGTM.

hicqu · 2018-04-20T05:26:48Z

src/pd/pd.rs

@@ -486,20 +595,29 @@ impl<T: PdClient> Runner<T> {
        self.is_hb_receiver_scheduled = true;
    }

+    fn maybe_handle_reconnect(&mut self) {
+        if self.invalidate_cache.swap(false, Ordering::Relaxed) {


I prefer Ordering::SeqCst to save other's brain.

Relaxed means that there is no memory ordering problem, so I think Relaxed is more clear.

Hoverbear · 2018-05-04T05:31:46Z

🙄 The build failed because of the Rust version upgrade having a weird Rustfmt incompatability. Please trigger a rebuild without the cache @overvenus .

overvenus · 2018-05-04T09:03:03Z

@Hoverbear Never mind, we are not going to merge this PR since PD works smoothly even if there are many region heartbeats.

Hoverbear · 2018-05-04T21:05:30Z

@overvenus So should we close this then? Or is it just delayed for now?

overvenus · 2018-05-05T03:07:37Z

Let's close it.

In tests, the mock pd-server(TestPdClient::Cluster) does not push region heartbeat responses. This PR try to simulate pushing by polling responses in the background. It's part of region heartbeat cache tikv#2701, for fixing tests.

overvenus requested review from siddontang, BusyJay and disksing January 19, 2018 07:23

overvenus added the WIP label Jan 19, 2018

disksing reviewed Jan 21, 2018

View reviewed changes

This was referenced Feb 9, 2018

tests: poll HeartbeatResponse #2755

Merged

pd: handle reconnect #2757

Merged

overvenus force-pushed the region-heartbeat-cache branch from d52b4b9 to 0616758 Compare February 11, 2018 08:09

overvenus removed the WIP label Feb 11, 2018

overvenus force-pushed the region-heartbeat-cache branch from 0616758 to d2921d8 Compare February 11, 2018 08:10

disksing mentioned this pull request Feb 24, 2018

Add background worker to check regions' status. tikv/pd#962

Closed

overvenus force-pushed the region-heartbeat-cache branch from d2921d8 to 23f794c Compare February 25, 2018 14:45

overvenus force-pushed the region-heartbeat-cache branch from 23f794c to b54cd7b Compare March 19, 2018 11:10

*: caching region heartbeats

120c296

overvenus force-pushed the region-heartbeat-cache branch from b54cd7b to 120c296 Compare March 22, 2018 07:22

overvenus requested a review from nolouch March 22, 2018 07:26

huachaohuang reviewed Mar 23, 2018

View reviewed changes

overvenus added 2 commits March 27, 2018 14:59

Address comments

1f42794

Merge branch 'master' into region-heartbeat-cache

d80aa83

nolouch reviewed Mar 27, 2018

View reviewed changes

hicqu reviewed Mar 27, 2018

View reviewed changes

overvenus added 4 commits March 29, 2018 15:59

Address comments

ba3574e

Merge branch 'master' into region-heartbeat-cache

a675e9b

Merge branch 'master' of github.com:pingcap/tikv into region-heartbea…

19b2a9a

…t-cache

Address comments

5b8f838

nolouch reviewed Apr 8, 2018

View reviewed changes

huachaohuang reviewed Apr 13, 2018

View reviewed changes

hicqu reviewed Apr 19, 2018

View reviewed changes

src/pd/pd.rs Show resolved Hide resolved

overvenus added 2 commits April 19, 2018 23:34

Merge branch 'master' into region-heartbeat-cache

807884c

Address comments

fff4a37

hicqu reviewed Apr 20, 2018

View reviewed changes

overvenus added the DNM label May 4, 2018

overvenus closed this May 5, 2018

overvenus deleted the region-heartbeat-cache branch January 16, 2020 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store: cache region heartbeat #2701

store: cache region heartbeat #2701

overvenus commented Jan 19, 2018

disksing Jan 21, 2018

disksing commented Jan 21, 2018

zhangjinpeng87 commented Jan 23, 2018 •

edited

overvenus commented Feb 27, 2018

huachaohuang Mar 23, 2018

huachaohuang Mar 23, 2018

overvenus Mar 27, 2018

huachaohuang Mar 23, 2018

overvenus commented Mar 27, 2018

nolouch Mar 27, 2018

hicqu Mar 27, 2018

overvenus Mar 27, 2018

hicqu Mar 27, 2018

overvenus commented Apr 2, 2018

nolouch left a comment

overvenus commented Apr 9, 2018

huachaohuang Apr 13, 2018

overvenus Apr 16, 2018

huachaohuang Apr 16, 2018

hicqu Apr 19, 2018

hicqu Apr 19, 2018

hicqu Apr 19, 2018

overvenus Apr 20, 2018

hicqu commented Apr 19, 2018

hicqu Apr 20, 2018

overvenus Apr 20, 2018

Hoverbear commented May 4, 2018

overvenus commented May 4, 2018

Hoverbear commented May 4, 2018

overvenus commented May 5, 2018

store: cache region heartbeat #2701

store: cache region heartbeat #2701

Conversation

overvenus commented Jan 19, 2018

Choose a reason for hiding this comment

disksing commented Jan 21, 2018

zhangjinpeng87 commented Jan 23, 2018 • edited

overvenus commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overvenus commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overvenus commented Apr 2, 2018

nolouch left a comment

Choose a reason for hiding this comment

overvenus commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoverbear commented May 4, 2018

overvenus commented May 4, 2018

Hoverbear commented May 4, 2018

overvenus commented May 5, 2018

zhangjinpeng87 commented Jan 23, 2018 •

edited