Minor issues in overlord

[empijei]

This is just some comments from an initial general review

Package structure

Packages should be named after their functions but the directory tree should reflect dependencies. The overlord/interests package is used by packages outside overlord. Since it is a shared util it might be worth considering moving it one level up to highlight what is shared across packages.

Overlord

Naming convention

File: ./overlord/overlord_test.go: underscores in names are discouraged.

Implement a deadline

Line: ./overlord/overlord.go:116: // TODO(paradoxengine): most likely, a deadline here?

A dummy code (untested) to do it

    var interests []*mappedInterest
    for name, m := range minions {
        // TODO(paradoxengine): most likely, a deadline here?
        var intResp *pb.ListInitialInterestsResponse
        var err error
        done := make(chan struct{})
        go func(){
            intResp, err = m.ListInitialInterests(ctx, &mpb.ListInitialInterestsRequest{})
            close(done)
        }()
        select {
            case <- time.After(5*time.Second):
                // handle timeout
                continue
            case <- done:
        }
        if err != nil {
            return nil, err
        }
        for _, v := range intResp.GetInterests() {
            interests = append(interests, &mappedInterest{
                interest: v,
                minion:   name,
            })
        }
    }
    return interests, nil

copy is a shallow copy

Line: ./overlord/overlord.go:143: copy(scanState.interests, s.initialInterests)

Beware this is a shallow copy and might behave weirdly if the copied slice elements are then modified

Potential race condition

grpc.Serve might spawn several goroutines at the same time, so it might be worth to protect some calls with a mutex (add a --gotsan or --race flag during tests to enable the race detector)

For example the following field is both written and read after server initialization: Line: ./overlord/overlord.go:34: scans map[string]state

Reads: overlord/overlord.go:161: scan, ok := s.scans[req.GetScanId()] overlord/overlord.go:176: scan, ok := s.scans[req.GetScanId()]

Write: overlord/overlord.go:150: s.scans[scan.ScanId] = scanState

Native go maps are not concurrent safe, maybe protect it with a sync.RWMutex or use a sync.Map instead.

Nits:

Line:./overlord/overlord_test.go:90:// TOOD(paradoxengine): the overlord still needs plenty of unit tests typo on TODO. Line:./overlord/overlord_test.go:90:// TOOD(paradoxengine): the overlord still needs plenty of unit tests maybe take a look at the cover tool.

[empijei]

@paradoxengine you might want to take a look at this and close it as this is due to a misunderstanding.

[paradoxengine]

This is actually pretty awesome and I'm going to address the stuff in it. Thanks!

[paradoxengine]

Looking at the notes: 0) Good point. I'll do it. 1) I believe the _test.go syntax is pretty standard? Seems to be what gazelle uses to know what is a test for the bazel test targets. 2) deadline. UHM. Shouldn't I just be able to derive a deadline from the context, by extending the ctx (which is background at this point)? I'd expect everything to be automagically handled then. 3) copy shallow - yep, that's WAI, i'm ok having more interests added/removed if we change the ones in the overlord. Is that uberconfusing? 4) I'm holding off on this because state is being fully revamped (there is a pull request out now). 5) Ah! Fixed. Yep, aware of the coverage instrumentation.

[empijei]

1. for filenames, not for function names that are in that file.
2. If you derive a deadline from context you still have to use a select statement to listen for timeout just use <- ctx.Done() as a case.
3. As long as that's what you intended to do that's fine.
4. ack

[paradoxengine]

1. Oh so this is actually about the names of the test functions. What's the practice there? I really like to have methodundertest_condition_expectedResults. Please don't tell me the convention is to just have a single name like testStuff. 2- Ah good point. Let me work on it.
2. ack.

[empijei]

1. The convention is TestFunctionBeingTestedCondition for tests and BenchmarkFunctionBeingBenchmarkedSizeOfTheBenchmark, e.g. TestDoMyStuffHeavyLoad, TestMyFuncCanCrash, TestMyFuncConcurrently, BenchmarkDoTheThing1000, BenchmarkDoTheThingConcurrently10