Reviewed 9 of 9 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 5 of 5 files at r4, 1 of 1 files at r5.
Reviewable status: 2 of 10 files reviewed, 6 unresolved discussions (waiting on @oncilla and @scrye)

go/lib/discovery/topofetcher/fetcher.go, line 33 at r2 (raw file):

type Callbacks struct {
	// Raw is called with the raw body from the discovery service response and the parsed topology.
	Raw func(common.RawBytes, *topology.Topo)

Isn't there a large amount of overlap between the content of the two arguments? Why are they both needed?

go/lib/infra/modules/idiscovery/idiscovery.go, line 136 at r5 (raw file):

}

// startPeriodicFetcherWriter starts a runner that periodically fetches the topology.

There's quite a bit of duplication here that might become awkward to maintain. I'm wondering if we can halve the number of types/implementations.

Is there a reason why the periodic runner/fetcher task of a dynamic topology can't have the same behavior/implementation as the static one? Sure, it's never useful to dump the dynamic to disk, so callers will never set the Filename field.

Would something like this work?

go/lib/infra/modules/idiscovery/idiscovery.go, line 230 at r5 (raw file):

// NewWriteFetcher creates a periodic.Task that fetches the topology from the discovery
// service and calls the provided handler on the received topology. If the handler indiacates

s/indiacates/indicates.

go/lib/infra/modules/idiscovery/idiscovery.go, line 232 at r5 (raw file):

// service and calls the provided handler on the received topology. If the handler indiacates
// an update, the topology is written to filename.
func NewWriteFetcher(handler TopoHandler, params discovery.FetchParams, filename string,

WriteFetcher parses oddly (it sounds like it fetches writes).

Maybe NewWritingFetcher?

Or, for such a specialized object, we can bite the bullet and go full Java with NewFetchThenWriteTask. Overall in the codebase it's going to be called rarely, so a bit of extra verbosity is not that big of an issue.

Reviewed 1 of 1 files at r6.
Reviewable status: 2 of 10 files reviewed, 6 unresolved discussions (waiting on @oncilla and @scrye)

Reviewable status: 2 of 10 files reviewed, 5 unresolved discussions (waiting on @scrye)

go/lib/discovery/topofetcher/fetcher.go, line 33 at r2 (raw file):

We want to avoid the callback from having to re-parse the topology unnecessarily.

go/lib/infra/modules/idiscovery/idiscovery.go, line 136 at r5 (raw file):

Refactored to reduced code duplication.
However, the two different types for StaticConfig and FetchConfig makes initialization cleaner IMO.

go/lib/infra/modules/idiscovery/idiscovery.go, line 230 at r5 (raw file):

Done.

go/lib/infra/modules/idiscovery/idiscovery.go, line 232 at r5 (raw file):

Removed during refactor.

Reviewed 4 of 9 files at r7, 1 of 1 files at r12.
Reviewable status: 3 of 10 files reviewed, all discussions resolved (waiting on @scrye)

Reviewed 4 of 9 files at r7.
Reviewable status: 3 of 10 files reviewed, all discussions resolved (waiting on @scrye)

Reviewed 1 of 1 files at r8, 5 of 5 files at r9, 1 of 1 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved