-
Notifications
You must be signed in to change notification settings - Fork 302
fleetd:registry: improve Units() performance remove extra loops when fetching Jobs list from etcd #1515
fleetd:registry: improve Units() performance remove extra loops when fetching Jobs list from etcd #1515
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,11 @@ | |
package functional | ||
|
||
import ( | ||
"fmt" | ||
"io/ioutil" | ||
"path" | ||
"reflect" | ||
"sort" | ||
"strings" | ||
"testing" | ||
|
||
|
@@ -404,3 +407,63 @@ func TestUnitStatus(t *testing.T) { | |
stdout, stderr) | ||
} | ||
} | ||
|
||
// TestListUnitFilesOrder simply checks if "fleetctl list-unit-files" returns | ||
// an ordered list of units | ||
func TestListUnitFilesOrder(t *testing.T) { | ||
cluster, err := platform.NewNspawnCluster("smoke") | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
defer cluster.Destroy() | ||
|
||
m, err := cluster.CreateMember() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
_, err = cluster.WaitForNMachines(m, 1) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
// Combine units | ||
var units []string | ||
for i := 1; i <= 20; i++ { | ||
unit := fmt.Sprintf("fixtures/units/hello@%02d.service", i) | ||
stdout, stderr, err := cluster.Fleetctl(m, "submit", unit) | ||
if err != nil { | ||
t.Fatalf("Failed to submit a batch of units: \nstdout: %s\nstder: %s\nerr: %v", stdout, stderr, err) | ||
} | ||
units = append(units, unit) | ||
} | ||
|
||
// make sure that all unit files will show up | ||
_, err = cluster.WaitForNUnitFiles(m, 20) | ||
if err != nil { | ||
t.Fatal("Failed to run list-unit-files: %v", err) | ||
} | ||
|
||
stdout, _, err := cluster.Fleetctl(m, "list-unit-files", "--no-legend", "--fields", "unit") | ||
if err != nil { | ||
t.Fatal("Failed to run list-unit-files: %v", err) | ||
} | ||
|
||
outUnits := strings.Split(strings.TrimSpace(stdout), "\n") | ||
|
||
var sortable sort.StringSlice | ||
for _, name := range units { | ||
n := path.Base(name) | ||
sortable = append(sortable, n) | ||
} | ||
sortable.Sort() | ||
|
||
var inUnits []string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is weird, why do you create another slice? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for this just to reflect how units are sorted and where sorted! since inUnits are sent using a counter which does not reflect how units are sorted and returned from etcd and golang. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and to follow up with DeepEqual() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically I also moved the logic from the Units() fleetd side to put it here in these lines of the functional test to ensure that Units() returns the same result without these lines.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite sure I follow. But I meant, you can just go directly to a slice: http://play.golang.org/p/xhsmv-XHSK There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see ;-D, thank you next time fore sure ;-) |
||
for _, name := range sortable { | ||
inUnits = append(inUnits, name) | ||
} | ||
|
||
if !reflect.DeepEqual(inUnits, outUnits) { | ||
t.Fatalf("Failed to get a sorted list of units from list-unit-files") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,7 @@ func (r *EtcdRegistry) Schedule() ([]job.ScheduledUnit, error) { | |
func (r *EtcdRegistry) Units() ([]job.Unit, error) { | ||
key := r.prefixed(jobPrefix) | ||
opts := &etcd.GetOptions{ | ||
// We need Job Units to be sorted | ||
Sort: true, | ||
Recursive: true, | ||
} | ||
|
@@ -117,7 +118,7 @@ func (r *EtcdRegistry) Units() ([]job.Unit, error) { | |
return unit | ||
} | ||
|
||
uMap := make(map[string]*job.Unit) | ||
units := make([]job.Unit, 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you are giving the map an initial capacity, why not the slice as well?... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the map is local collected, the slice is not. |
||
for _, dir := range res.Node.Nodes { | ||
u, err := r.dirToUnit(dir, unitHashLookupFunc) | ||
if err != nil { | ||
|
@@ -127,18 +128,8 @@ func (r *EtcdRegistry) Units() ([]job.Unit, error) { | |
if u == nil { | ||
continue | ||
} | ||
uMap[u.Name] = u | ||
} | ||
|
||
var sortable sort.StringSlice | ||
for name, _ := range uMap { | ||
sortable = append(sortable, name) | ||
} | ||
sortable.Sort() | ||
|
||
units := make([]job.Unit, 0, len(sortable)) | ||
for _, name := range sortable { | ||
units = append(units, *uMap[name]) | ||
units = append(units, *u) | ||
} | ||
|
||
return units, nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I had thought that it would be redundant to call
"fleetctl list-unit-files"
again. But after looking into the cluster operation deeper, I can confirm that actuallycluster.WaitForNUnitFiles()
does not return a sorted map. (Hmm, not exactly what I expected - scratching my head.) Of course we could make it return a sorted map, if we would really want it. But for now I think it's fine to simply call"fleetctl list-unit-files"
again like this, as this functional test is not in a critical hot-path.So LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Thanks!