update to make all analysis take place in func() #1508
Conversation
by making analysis take place inside a func() a closure can be used to encapsulate all the index specific details. this allows the index API to not know about scorch vs upsidedown analysis structures
|
Currently this is stacked on top of the other change, as that the plugin interface moving is related (we can re-target it to fix-circular-deps if we merge make-plugin-interface-local first) Expect this to fail, as it needs the zap changes to satisfy the new |
index/scorch/scorch.go
Outdated
| @@ -335,16 +335,19 @@ func (s *Scorch) Batch(batch *index.Batch) (err error) { | |||
| go func() { | |||
| for _, doc := range batch.IndexOps { | |||
| if doc != nil { | |||
| aw := index.NewAnalysisWork(s, doc, resultChan) | |||
| doc := doc | |||
There was a problem hiding this comment.
not sure whether I followed this doc := doc logic ?
There was a problem hiding this comment.
So, this is the classic Go range variable capture issue. The doc declared in the scope of the range statement is a single variable which changes each iteration of the loop. Because doc is type interface, it's essentially a pointer, then these are passed via channels, and accessed later, by which time the contents of the pointer has changed to some later iteration.
The doc := doc is declaring a new variable inside the loop iteration, which copies the doc from the range statement. This ensures the doc that is accessed later is a unique variable, not shared.
It is fundamentally the exact same as this line in upsidedown:
bleve/index/upsidedown/upsidedown.go
Line 824 in c3457bb
| @@ -41,7 +41,7 @@ type Plugin interface { | |||
| Version() uint32 | |||
|
|
|||
| // New takes a set of AnalysisResults and turns them into a new Segment | |||
| New(results []*index.AnalysisResult) (segment.Segment, uint64, error) | |||
| New(results []index.Document) (segment.Segment, uint64, error) | |||
There was a problem hiding this comment.
Perhaps the above comment needs updates too?
also update comment, per review feedback
selects latest versions compatible with these changes
* move the plugin interface into scorch * rename scorch Plugin to SegmentPlugin and related functions * update to make all analysis take place in func() (#1508) * update to make all analysis take place in func() by making analysis take place inside a func() a closure can be used to encapsulate all the index specific details. this allows the index API to not know about scorch vs upsidedown analysis structures * rewrite variable declaration for clarity also update comment, per review feedback * update to latest versions selects latest versions compatible with these changes
by making analysis take place inside a func() a
closure can be used to encapsulate all the index
specific details. this allows the index API to not
know about scorch vs upsidedown analysis structures