refactor(controller): use internal/targetdiff for GetChangedTargets#174
refactor(controller): use internal/targetdiff for GetChangedTargets#174xytan0056 wants to merge 7 commits into
Conversation
Replace the open-coded diff/classify/BFS logic in getchangedtargets.go with internal/targetdiff.Compare. The controller now decodes each graph stream into a semantic targetdiff.Graph, compares, and re-maps the result into canonical wire IDs. Behavior is unchanged.
Extract the cache-serve, concurrent graph-fetch, and fire-and-forget cache-write phases into serveChangedTargetsFromCache, fetchTargetGraphs, and cacheComparedTargets. GetChangedTargets drops from ~280 to ~85 lines and now reads as a linear pipeline. Behavior is unchanged.
| logger.Info("GetChangedTargets: Cache hit, streaming from storage", | ||
| zap.Duration("cache_read_duration", cacheReadDuration), | ||
| ) | ||
| scope.Counter("changed_targets_cache_hit").Inc(1) |
There was a problem hiding this comment.
keep the old metric recording here until the metrics package is ready
xytan0056
changed the title
| // GetChangedTargets returns the changed targets between two revisions. If the | ||
| // client disconnects, the stream's context is cancelled and the function | ||
| // returns with context.Canceled. | ||
| func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (retErr error) { |
There was a problem hiding this comment.
now the call flow is
GetChangedTargets
-> serveChangedTargetsFromCache
-> fetchTargetGraphs for both revs
-> compareTargetGraphs
-> transpose
-> cacheComparedTargets
| if err != nil { | ||
| logger.Error("GetChangedTargets: Failed to read revision treehash", zap.Error(err)) | ||
| return common.WithReason(failureReasonTreehashRead, common.ErrorTypeInfra, err) | ||
| served, err := c.serveChangedTargetsFromCache(ctx, scope, logger, request, stream, maxDist, start) |
There was a problem hiding this comment.
Just to confirm, with the refactoring to scope and logger, we won't need to provide this as input everywhere (Initially I was wondering why aren't these set as fields on the controller where we can do c.scope(), c.logger()
There was a problem hiding this comment.
yes, these shouldn't have been passed as arguments -- will be further refactored once metrics package is ready to use. We will extract scope from ctx.
For now, because the ctx is not there yet, we need to pass them in like this, we cannot share the scope from controller struct like c., because each of these methods technically are child scope, from parent, which doesn't carry some enriched infomation.
| // Cache the computed result concurrently so it doesn't block the stream send. | ||
| c.cacheComparedTargets(logger, request, changedTargetsResponses) |
There was a problem hiding this comment.
IMO it should be the responsibility of the top level method to decide whether to cache concurrently (In a goroutine or not).
The underlying helper method should just do the caching comparing logic, not make decision on whether it should be in a goroutine or not
There was a problem hiding this comment.
oh this is not comparing, this is to fire and forget function to cache the ctc result. so that the next request for the same treehash compare could return fast from cache
So the "goroutine or not" decision is supposed to be determined in the underlying function (reason being use separte appCtx + best effort + caller not changing response, these re nothing to do with caller)
…rify cache flow Move the compare_duration timer and completion log into compareTargetGraphs so it is self-contained like fetchTargetGraphs, rather than split across the caller. Error classification stays at the handler per the error-classification contract. Also split the cache fast-path return into explicit served/miss/fail branches for readability.
2 participants
Now that we have
internal/compare.go, move the diff/classify/BFS logic ingetchangedtargets.gotointernal/targetdiff.Compare. The controller decodes each graph stream into a semantic graph, compares, and re-maps. Behavior not changed.Test
make test
Manually tested locally