Add basic resharding record migration logic for new cluster (no GC yet) #21
Conversation
dns/httpapi.go
Outdated
| @@ -119,6 +196,110 @@ func serveHTTPAPI(store *dnsStore, port int, confChangeC chan<- raftpb.ConfChang | |||
| w.WriteHeader(http.StatusNoContent) | |||
| }) | |||
|
|
|||
| // TODO: confChange requests | |||
There was a problem hiding this comment.
Remove this TODO: confChange comments. Instead, add a bit more comment about how /addcluster is called (see the format I had for other routes)
dns/httpapi.go
Outdated
| w.WriteHeader(http.StatusNoContent) | ||
| }) | ||
|
|
||
| // TODO: confChange requests |
There was a problem hiding this comment.
comment ditto (ditto means similar to above)
| RecordsMap dnsRRTypeMap | ||
| } | ||
|
|
||
| router.HandleFunc("/getrecord", func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
ditto for usage comments (for example, the body is a number for index, etc.)
dns/httpapi.go
Outdated
|
|
||
| body, err := ioutil.ReadAll(r.Body) | ||
| if err != nil { | ||
| log.Printf("Cannot read /addcluster body: %v\n", err) |
There was a problem hiding this comment.
s/addcluster/getrecord
| log.Fatal(err) | ||
| } | ||
| // update cluster info in dnsStore | ||
| store.config = jsonClusters |
There was a problem hiding this comment.
read/write lock the store. Similar for many places below.
(Be careful, Go locks are not reentrant and thus can self deadlock)
dns/httpapi.go
Outdated
| return string(t) | ||
| } | ||
|
|
||
| // func (h *dnsHTTPAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
Remove this commented section, since membership update is already implemented.
| log.Println("Domain is ", domain) | ||
| // log.Println(store.lookup.LocateKey([]byte(domain)).String()) | ||
| if shouldMigrate(domain, store) { | ||
| for _, rrs := range store.store[domainNames[index]] { |
There was a problem hiding this comment.
Need to be careful with lock granularity (to avoid blocking normal DNS read requests serving)
Also this API does not seem to do streaming (and thus response body will be huge with large JSON parsing overhead), but I guess I will also let it pass for initial implementation. Leave a comment to warn about this problem though.
There was a problem hiding this comment.
Are locks needed here? The assumption (implied by the new cluster node's migration logic) is that writes at hash servers are disabled at this point, so all operations should be reads.
There was a problem hiding this comment.
Better still grab the read lock. It is fine to have multiple read locks held anyways.
There was a problem hiding this comment.
Added read lock around the entire if statement since shouldMigrate() also accesses store.
| @@ -48,6 +49,10 @@ type dnsStore struct { | |||
| // for sending http Cache requests | |||
| cluster []string | |||
| id int | |||
| // for adding new cluster and migration | |||
| config []jsonClusterInfo | |||
There was a problem hiding this comment.
This differs from what I originally thought of (with state temporarily maintained in dedicated thread instead of becoming part of the store state), but I'll also probably give this a pass for initial attempt.
There was a problem hiding this comment.
Yeah that definitely sounds better, no need to change the store state.
| return | ||
| } | ||
| // this is async, might cause too much traffic | ||
| store.ProposeAddRR(rrString) |
There was a problem hiding this comment.
Add a small comment to explain why locks are not needed here (due to proposals done through the channel)
| migrate := flag.Bool("migrate", false, "need to coordinate migration") | ||
| // include the port in hash server ip addr | ||
| hashServer := flag.String("hash", "", "the ip address of a hash server to help with migration") | ||
| config := flag.String("config", "", "the path to the json file containing the config for this cluster") |
There was a problem hiding this comment.
Add a comment to explain the format of the config (mostly how it is different from hash_server config)
kevinkassimo
changed the title
|
Also I believe this code is not yet tested? (seems the logic for disabling/reenabling writes is not actually implemented). Be sure to do a minimum test before final merge. |
| } | ||
|
|
||
| // update cluster info in dnsStore | ||
| store.clusters = intoClusterMap(jsonClusters) |
| Hasher: hasher{}, | ||
| } | ||
| log.Println("Received cluster update, setting new config") | ||
| store.lookup = consistent.New(nil, cfg) |
| log.Println("Received cluster update, setting new config") | ||
| store.lookup = consistent.New(nil, cfg) | ||
| for _, clusterJSON := range store.config { | ||
| store.lookup.Add(clusterToken(clusterJSON.ClusterToken)) |
| log.Println("Domain is ", domain) | ||
| // log.Println(store.lookup.LocateKey([]byte(domain)).String()) | ||
| if shouldMigrate(domain, store) { | ||
| for _, rrs := range store.store[domainNames[index]] { |
There was a problem hiding this comment.
Better still grab the read lock. It is fine to have multiple read locks held anyways.
Basic function for migrating RRs from old clusters to the new cluster and updates config at hash servers.
Example command for a new cluster to start migration:
sudo ./dns_server -migrate -hash "172.31.30.97" -config ./new_config.json -port 9121 -cluster "http://172.31.19.166:12379"Some functions are not implemented and I will discuss these functions with who picks these tasks: