Coordinator multi-fetch fixes #989
Conversation
prateek
force-pushed
the
prateek/coordinator-panic-fixes
branch
from
832cfba
to
21cefe2
Compare
prateek
force-pushed
the
prateek/coordinator-panic-fixes
branch
from
21cefe2
to
f88d7b9
Compare
|
|
||
| for _, iter := range iters { | ||
| id := iter.ID().String() | ||
| r.dedupeMap[id] = append(r.dedupeMap[id], multiResultSeries{ |
There was a problem hiding this comment.
Cant this panic if the exiting value is a nil slice?
There was a problem hiding this comment.
talked offline, appending to nil slice is kosher
There was a problem hiding this comment.
We're always going to alloc a map and an array per entry in the map now?
| if exists && existing.attrs.Resolution <= attrs.Resolution { | ||
| // Already exists and resolution of result we are adding is not as precise | ||
| func (r *multiResult) dedupe() { | ||
| for id, serieses := range r.dedupeMap { |
There was a problem hiding this comment.
serieses --> seriesList or something
| return nil, noop, fmt.Errorf("no namespaces configured") | ||
| } | ||
|
|
||
| pools, err := namespaces[0].Session().IteratorPools() |
There was a problem hiding this comment.
maybe leave a todo to find a way where we're not always ignoring pools from the other namespaces
There was a problem hiding this comment.
talked offline, will comeback for this.
| attrs: r.dedupeFirstAttrs, | ||
| } | ||
| } | ||
| r.dedupeMap = make(map[string][]multiResultSeries, len(iters)) |
There was a problem hiding this comment.
Hm now we are always going to be making the dedupe map (regardless of if we only hit a single namespace)?
|
Thanks for fixing this, I should have had better tests to catch this. Ignore the comments I posted, I can revisit the perf, to avoid de-dupe map if we don't need it, etc. |
No description provided.