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

*: simplify raft cluster. #78

Merged
merged 5 commits into from
Apr 19, 2016
Merged

*: simplify raft cluster. #78

merged 5 commits into from
Apr 19, 2016

Conversation

siddontang
Copy link
Contributor


func (s *testClusterCacheSuite) SetUpSuite(c *C) {
s.svr = newTestServer(c, s.getRootPath())

Copy link
Member

Choose a reason for hiding this comment

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

Remove empty line.

// cluster 1 -> /raft/1, value is metapb.Cluster
// cluster 2 -> /raft/2
// cluster 1 -> /1/raft, value is metapb.Cluster
// cluster 2 -> /1/raft
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment should be /2/raft?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

@qiuyesuifeng
Copy link
Contributor

LGTM

@@ -21,6 +21,7 @@ var (
leaderLease = flag.Int64("lease", 3, "Leader lease time (second)")
logLevel = flag.String("L", "debug", "log level: info, debug, warn, error, fatal")
pprofAddr = flag.String("pprof", ":6060", "pprof HTTP listening address")
clusterID = flag.Uint64("cluster-id", 0, "Cluster ID")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe another default clusterID since 0 is for test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check 0 and panic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think make the default value to be 1 is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

em, 1 seems strange, I prefer 0 if 1.

We can log a warning if we use 0. Btw for most cases, we use different root path not "/pd", so it is ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine.

@disksing
Copy link
Contributor

LGTM

@siddontang
Copy link
Contributor Author

PTAL @ngaut

@ngaut
Copy link
Member

ngaut commented Apr 19, 2016

LGTM

@siddontang siddontang merged commit f467c7d into master Apr 19, 2016
@siddontang siddontang deleted the siddontang/simplify-cluster branch April 19, 2016 09:23
lhy1024 pushed a commit to lhy1024/pd that referenced this pull request May 17, 2023
* added get region by keyspace api

Signed-off-by: David <[email protected]>

* fix encoding

Signed-off-by: David <[email protected]>

* addd pd-ctl

Signed-off-by: David <[email protected]>

* change description

Signed-off-by: David <[email protected]>

* change pd-ctl command to add id

Signed-off-by: David <[email protected]>

* check keyspace id before fetch

Signed-off-by: David <[email protected]>

---------

Signed-off-by: David <[email protected]>
Signed-off-by: lhy1024 <[email protected]>
rleungx pushed a commit to rleungx/pd that referenced this pull request Jun 5, 2023
* added get region by keyspace api

Signed-off-by: David <[email protected]>

* fix encoding

Signed-off-by: David <[email protected]>

* addd pd-ctl

Signed-off-by: David <[email protected]>

* change description

Signed-off-by: David <[email protected]>

* change pd-ctl command to add id

Signed-off-by: David <[email protected]>

* check keyspace id before fetch

Signed-off-by: David <[email protected]>

---------

Signed-off-by: David <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants