*: Add a new way to store metadata of regions #1237

nolouch · 2018-09-06T17:13:54Z

What problem does this PR solve?

This Version is used for testing first, not completed.

What is changed and how it works?

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

siddontang · 2018-09-07T05:23:27Z

em, why introduce LevelDB, do we really need it?

nolouch · 2018-09-12T08:36:40Z

@siddontang we need load regions from regions to speed up recover router of the regions.
PTAL @disksing @rleungx @liukun4515

nolouch · 2018-09-13T09:14:39Z

/rebuild

rleungx · 2018-09-17T03:10:29Z

server/region_syncer.go

+		return err
+	}
+
+	cc, err := grpc.Dial(u.Host, grpc.WithInsecure(), grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(8*1024*1024)))


better to use a constant?

rleungx · 2018-09-17T03:11:09Z

server/region_syncer.go

+	s.Unlock()
+}
+
+func (s *regionSyncer) stopSyncerWithLeader() {


maybe stopSyncWithLeader?

rleungx · 2018-09-17T03:11:32Z

server/region_syncer.go

+	s.wg.Wait()
+}
+
+func (s *regionSyncer) statSyncerWithLeader(addr string) error {


Do you mean start*?

rleungx · 2018-09-17T03:13:14Z

server/region_syncer.go

+	}
+	u, err := url.Parse(addr)
+	if err != nil {
+		return err


How about errors.WithStack(err)?

rleungx · 2018-09-17T03:19:59Z

server/core/levedb_kv.go

+		count++
+	}
+	iter.Release()
+	//iter.Error()


What does this comment mean?

rleungx · 2018-09-17T03:31:20Z

server/core/kv_base.go

@@ -27,6 +28,13 @@ type KVBase interface {
 	Delete(key string) error
 }

+// RegionKV used to save region metadata.


rleungx · 2018-09-17T03:34:11Z

server/config.go

@@ -633,6 +635,12 @@ func (s SecurityConfig) ToTLSConfig() (*tls.Config, error) {
 	return tlsConfig, nil
 }

+// PDServerConfig is the configuration for pd server.
+type PDServerConfig struct {
+	//  EnableRegionStorage enable the independent region  storage.


enables and remove the extra space

rleungx · 2018-09-17T03:34:46Z

server/core/kv.go

+	DefaultBatchSize = 100
+)
+
+// WithLevelDBKV store the regions information in levelDB.


rleungx · 2018-09-17T03:35:42Z

server/core/kv.go

+	}()
+}
+
+// FlushRegion save the cache region to region kv storage.


rleungx · 2018-09-17T03:37:17Z

server/core/levedb_kv.go

+	db *leveldb.DB
+}
+
+// NewLeveldbKV to store regions information.


is used to store

rleungx · 2018-09-17T04:45:20Z

server/core/kv.go

+		}
+		kv.cacheSize = 0
+		kv.batchRegions = make(map[string]*metapb.Region, kv.batchSize)
+		return nil


rleungx · 2018-09-17T04:46:06Z

server/core/kv.go

+		kv.cacheSize = 0
+		kv.batchRegions = make(map[string]*metapb.Region, kv.batchSize)
+		return nil
+		//return saveProto(kv.regionKV, kv.regionPath(region.GetId()), region)


should be removed?

disksing · 2018-09-17T09:09:30Z

server/server.go

@@ -388,7 +393,8 @@ func (s *Server) bootstrapCluster(req *pdpb.BootstrapRequest) (*pdpb.BootstrapRe
 	}

 	log.Infof("bootstrap cluster %d ok", clusterID)
-
+	s.kv.SaveRegion(req.GetRegion())


No need to check error?

disksing · 2018-09-17T09:11:17Z

server/server.go

@@ -208,7 +209,8 @@ func (s *Server) startServer() error {

 	s.idAlloc = &idAllocator{s: s}
 	kvBase := newEtcdKVBase(s)
-	s.kv = core.NewKV(kvBase)
+	path := filepath.Join(s.cfg.DataDir, "leveldb")


How about making it something like "region-meta"?

disksing · 2018-09-17T09:13:48Z

server/grpc_service.go

+			return status.Errorf(codes.FailedPrecondition, "mismatch cluster id, need %d but got %d", s.clusterID, request.GetHeader().GetClusterId())
+		}
+		switch request.GetTp() {
+		case 1:


What is the Tp used for? I think better to define it as enum in proto file.

disksing · 2018-09-17T09:16:22Z

server/core/kv.go

+			if !isFlush {
+				continue
+			}
+			kv.FlushRegion()


Check the error?

disksing · 2018-09-17T09:19:55Z

server/cluster.go

+		running:      false,
+		clusterID:    clusterID,
+		clusterRoot:  s.getClusterRootPath(),
+		clients:      make(map[string]pdpb.PDClient),


disksing · 2018-09-17T09:40:28Z

server/cluster.go

+				Header: &pdpb.ResponseHeader{ClusterId: c.s.clusterID},
+				Data:   data,
+			}
+			for _, stream := range c.regionSyncer.streams {


Read streams without lock?

disksing · 2018-09-17T09:48:13Z

server/region_syncer.go

+
+type regionSyncer struct {
+	sync.Mutex
+	streams map[string]pdpb.PD_SyncRegionsServer


I think streams can be managed in Server directly. As it is used for leader to sync regions to followers, while other fields are for sync regions with leader.

disksing · 2018-09-17T09:49:49Z

server/leader.go

+		log.Error("reload config failed:", err)
+		return
+	}
+	err = s.cluster.regionSyncer.statSyncerWithLeader(leader.GetClientUrls()[0])


Should not the first address. Maybe part of addrs can be accessed and the valid address is not in the first place.

disksing · 2018-09-17T09:51:40Z

server/core/kv.go

+	return nil
+}
+
+func (kv *KV) doGC() {


I don't think GC is a suitable name here.

disksing · 2018-09-17T09:52:25Z

server/core/kv.go

+}
+
+// FlushRegion save the cache region to region kv storage.
+func (kv *KV) FlushRegion() error {


Is it really necessary to save regions to levelDB in batches?

disksing · 2018-09-25T02:56:11Z

server/region_syncer.go

+		cancel()
+		return nil, err
+	}
+	err = client.Send(&pdpb.SyncRegionRequest{


Why do we need client streaming if it only sends one request?

we also use it to receive the response of newly updated regions from leader.

I guess server streaming will be enough.
rpc SyncRegions(~~stream~~ SyncRegionRequest) returns (stream SyncRegionResponse) {}

nolouch · 2018-09-26T05:28:17Z

PTAL @disksing @rleungx @liukun4515

rleungx · 2018-09-26T09:56:30Z

server/core/kv.go

+func NewKVProxy(defaultKV KVBase, regionKV *RegionKV) *KVProxy {
+	kv := &KVProxy{
+		defaultKV: defaultKV,
+		regionKV:  regionKV,


seem we don't need to initialize regionKV with nil here

rleungx · 2018-09-26T13:24:18Z

pkg/integration_test/region_syncer_test.go

+	}
+	// ensure flush to region kv
+	time.Sleep(3 * time.Second)
+	leaderServer.Stop()


check error?

rleungx · 2018-09-26T13:28:23Z

pkg/integration_test/region_syncer_test.go

+		regions = append(regions, core.NewRegionInfo(r, r.Peers[0]))
+	}
+	for _, region := range regions {
+		rc.HandleRegionHeartbeat(region)


check error?

rleungx · 2018-09-26T13:42:05Z

server/config.go

@@ -633,6 +635,12 @@ func (s SecurityConfig) ToTLSConfig() (*tls.Config, error) {
 	return tlsConfig, nil
 }

+// PDServerConfig is the configuration for pd server.
+type PDServerConfig struct {


Do we really need this? Can we put EnableRegionStorage into Config directly?

We use it to persist the config of PD-server.

What do you mean by persist to PD-server?

disksing · 2018-09-27T05:21:18Z

server/region_syncer.go

+	for _, sender := range s.streams {
+		err := sender.Send(regions)
+		if err != nil {
+			log.Error("region syncer send data meet error:", err)


Do we want to remove the stream from the map?

disksing · 2018-09-27T05:32:36Z

server/core/kv_base.go

@@ -26,7 +26,6 @@ type KVBase interface {
 	Save(key, value string) error
 	Delete(key string) error
 }
-


Why remove this line?

disksing · 2018-09-27T05:41:46Z

server/leader.go

+		if isSync {
+			s.cluster.regionSyncer.stopSyncWithLeader()
+		}
+	}()


if sync { startSyncWithLeader defer stopSyncWithLeader }

disksing · 2018-09-27T05:49:15Z

server/core/kv.go

+	isDefault atomic.Value
+}
+
+// NewKVProxy return a proxy of KV storage.


…just

liukun4515 · 2018-10-08T03:33:33Z

server/core/region_kv.go

+					continue
+				}
+				if err = kv.FlushRegion(); err != nil {
+					log.Info("flush regions error: ", err)


liukun4515 · 2018-10-08T03:47:51Z

server/core/region_kv.go

+// Close closes the kv.
+func (kv *RegionKV) Close() error {
+	kv.cancel()
+	return kv.db.Close()


should we flush the cached region before closing the leveldb?

rleungx · 2018-10-08T02:54:16Z

pkg/integration_test/region_syncer_test.go

+	c.Assert(leaderServer, NotNil)
+	loadRegions := leaderServer.server.GetRaftCluster().GetRegions()
+	c.Assert(len(loadRegions), Equals, regionLen)
+	cluster.Destroy()


maybe use defer here?

rleungx · 2018-10-08T03:15:50Z

server/core/kv.go

-				}
-			}
-		}
+// Flush flush the dirty region to storage.


rleungx · 2018-10-08T03:22:02Z

server/core/region_kv.go

+	defaultBatchSize = 100
+)
+
+// NewRegionKV return a kv storage that is used to save regions.


rleungx · 2018-10-08T03:29:41Z

server/core/region_kv.go

+	rangeLimit := maxKVRangeLimit
+	for {
+		key := regionPath(nextID)
+		res, err := kv.LoadRange(key, endKey, rangeLimit)


better to use startKey?

rleungx · 2018-10-08T03:48:48Z

server/server.go

-
+	err = s.kv.SaveRegion(req.GetRegion())
+	if err != nil {
+		log.Warnf("save the bootstrap region faild: %s", err)


s/faild/failed/g

liukun4515 · 2018-10-08T06:28:18Z

server/core/region_kv.go

+func (kv *RegionKV) SaveRegion(region *metapb.Region) error {
+	kv.mu.Lock()
+	defer kv.mu.Unlock()
+	if kv.cacheSize < kv.batchSize {


kv.cacheSize < kv.batchSize -1

kv.cacheSize < kv.batchSize means that the flush will be triggered until the size of batchRegions reach to batchSize+1

liukun4515

LGTM

disksing · 2018-10-08T09:05:11Z

server/core/levedb_kv.go

+func newLeveldbKV(path string) (*leveldbKV, error) {
+	db, err := leveldb.OpenFile(path, nil)
+	if err != nil {
+		return nil, err


Need to wrap the leveldb errors with call stack.

…ldb-adjust

disksing · 2018-10-09T03:21:27Z

PTAL @rleungx

rleungx

The rest LGTM.

rleungx · 2018-10-09T06:14:30Z

server/core/kv.go

@@ -35,11 +37,14 @@ const (
 const (
 	maxKVRangeLimit = 10000
 	minKVRangeLimit = 100
+	dirtyFlushTick  = time.Second


better to move it into region_kv.go?

rleungx · 2018-10-09T06:15:55Z

server/core/kv.go

-				}
-			}
-		}
+// Flush flushs the dirty region to storage.


…ldb-adjust

nolouch · 2018-10-10T08:24:03Z

/rebuild

nolouch · 2018-10-10T08:25:10Z

/run-all-tests

nolouch added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2018

nolouch removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2018

nolouch force-pushed the save-goleveldb branch 2 times, most recently from 92240fe to 4eaa00c Compare September 12, 2018 08:25

nolouch added 4 commits September 12, 2018 16:28

update kvproto

8aad56c

*: save meta information about region in levelDB

1a741f7

use stream to sync regions

abcb224

add switch

cbcd851

nolouch force-pushed the save-goleveldb branch from 473633f to cbcd851 Compare September 12, 2018 08:29

nolouch requested review from disksing, rleungx and liukun4515 September 12, 2018 08:36

nolouch changed the title *: save metadata of regions to leveldb *: Add a new way to store metata of regions Sep 12, 2018

nolouch changed the title *: Add a new way to store metata of regions *: Add a new way to store metadata of regions Sep 12, 2018

rleungx reviewed Sep 17, 2018

View reviewed changes

disksing reviewed Sep 17, 2018

View reviewed changes

nolouch added 2 commits September 21, 2018 16:34

update kvproto

d48d655

address comments

308cdde

nolouch force-pushed the save-goleveldb branch from 92da196 to 308cdde Compare September 25, 2018 02:41

disksing reviewed Sep 25, 2018

View reviewed changes

address comments

cb41f3d

rleungx reviewed Sep 26, 2018

View reviewed changes

address comments

9a0360e

nolouch force-pushed the save-goleveldb branch from db433ff to 9a0360e Compare September 27, 2018 03:30

disksing reviewed Sep 27, 2018

View reviewed changes

server/core/kv.go Outdated

isDefault atomic.Value

}

// NewKVProxy return a proxy of KV storage.

Copy link

Contributor

disksing Sep 27, 2018

Choose a reason for hiding this comment

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

returns

nolouch added 4 commits September 27, 2018 15:12

address comments

54187e2

add keepalive

ed35f84

Merge remote-tracking branch 'upstream/master' into save-goleveldb-ad…

da984d2

…just

use master kvproto

1241a98

liukun4515 reviewed Oct 8, 2018

View reviewed changes

rleungx reviewed Oct 8, 2018

View reviewed changes

nolouch added 2 commits October 8, 2018 14:10

address comments

9f31426

Merge branch 'master' into save-goleveldb

547dd57

liukun4515 reviewed Oct 8, 2018

View reviewed changes

address comments

f50e3ca

liukun4515 approved these changes Oct 8, 2018

View reviewed changes

Merge branch 'master' into save-goleveldb

155ebb5

disksing reviewed Oct 9, 2018

View reviewed changes

nolouch added 2 commits October 9, 2018 10:55

wrap the leveldb error

81508b9

Merge remote-tracking branch 'origin/save-goleveldb' into save-goleve…

4db127e

…ldb-adjust

disksing approved these changes Oct 9, 2018

View reviewed changes

Merge branch 'master' into save-goleveldb

1b02452

rleungx approved these changes Oct 9, 2018

View reviewed changes

nolouch added 2 commits October 9, 2018 15:19

address comments

454c27b

Merge remote-tracking branch 'origin/save-goleveldb' into save-goleve…

fa03500

…ldb-adjust

nolouch merged commit ef2bdf2 into tikv:master Oct 10, 2018

nolouch deleted the save-goleveldb branch October 10, 2018 08:55

overvenus mentioned this pull request Oct 17, 2018

Consider moving SyncRegions to another gRPC service #1276

Open

*: Add a new way to store metadata of regions #1237

*: Add a new way to store metadata of regions #1237

Conversation

nolouch commented Sep 6, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

siddontang commented Sep 7, 2018

nolouch commented Sep 12, 2018

nolouch commented Sep 13, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch commented Sep 26, 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

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

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

liukun4515 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disksing commented Oct 9, 2018

rleungx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch commented Oct 10, 2018

nolouch commented Oct 10, 2018

nolouch Sep 25, 2018 •

edited

Loading