feat(conflict): Init Tango-backed target-overlap conflict analyzer - #589
feat(conflict): Init Tango-backed target-overlap conflict analyzer#589manjari25 wants to merge 6 commits into
Conversation
|
|
| // TargetResolver resolves the set of build targets a batch affects. The | ||
| // production implementation translates the batch's changes into a Tango | ||
| // GetChangedTargets call; tests supply a fake. | ||
| type TargetResolver interface { |
There was a problem hiding this comment.
should this be an internal interface?
There was a problem hiding this comment.
Discussed offline,
we can restructure a bit to do something like this,
/submitqueue/extension/dependency/
/conflict/conflict.go
/conflict/all/*.go
/conflict/none/*.go
/conflict/fake/*.go
/resolver/resolver.go
/resolver/<impl>/<impl>.go
There was a problem hiding this comment.
Set up the tango resolver and analyzer impl in this PR. Will move over the existing ones in the next PR.
2cf9760 to
8b63965
比较
| // two batches when their changed build targets overlap. The targets a batch | ||
| // affects are resolved through an injected resolver.TargetResolver, whose | ||
| // production implementation calls the Tango service. | ||
| package tango |
There was a problem hiding this comment.
do we want to call tango? i think it's a generic targetanalyzer based?
There was a problem hiding this comment.
Good point. Making the change. Will call it targetanalyzer.
540b86c to
484dbcd
比较
| // Attributes carries backend-specific metadata the analyzer does not | ||
| // interpret today. Future consumers (e.g. conflict relaxation) can read | ||
| // keys like "distance" or "rule_type" without an interface change. | ||
| Attributes map[string]string |
There was a problem hiding this comment.
It might be inefficient for memory / GC but tempting to use for adhoc extensions.
Let's say we always have "distance" property for each target.
This means that each target structure will have a map and each map will have the same key "distance" (8 bytes * number_of_targets right away) and each distance value is a string to be converted to int.
Do we really need to abstract out target-based conflict analysis?
The option is to have a conflict analyzer that is specific to Tango API. If another build target based analyzer needs to be implemented, it will just become a separate extension with its own domain model.
There was a problem hiding this comment.
to have a conflict analyzer that is specific to Tango API
That's what I originally had - I made the changes to reflect this. I think we still need a lightweight interface that wraps the tango RPC because generated Go types aren't interchangeable due to different import paths.
There was a problem hiding this comment.
Interface is fine but it likely wants to be specific and type-safe.
@behinddwalls objections?
There was a problem hiding this comment.
Yes..we want to have specific types defined for this which are specific to tango. Plus, I am unsure how we enable conflict relaxation on top if we want to use any target related information. So another question I have on is the conflict interface as well.. wether that's right abstraction or not. Or should SQ define how to accept the target information and then run conflicts separately and be aware of target information so that we can store and use across different phases
Summary
conflict.Analyzerimplementation that detects conflicts betweenbatches based on build-target overlap, resolved through a
TargetResolverinterface whose production implementation will call Tango's
GetChangedTargetsin a follow-up PR.pathoverlap— takes identity-levelbatches, resolves targets internally, compares sets locally — but operates
at build-target granularity instead of file paths.
Test plan
make test,make lint,make fmt,make check-gazellepass