MB-61890 - Introducing config for zap layer#2066
Conversation
Thejas-bhat
requested review from
CascadingRadium,
Likith101,
abhinavdangeti,
metonymic-smokey and
moshaad7
metonymic-smokey
changed the title
Thejas-bhat
force-pushed
the
zapConfig
branch
from
de21e72 to
4030e93
Compare
Thejas-bhat
requested review from
capemox and
maneuvertomars
and removed request for
metonymic-smokey
Thejas-bhat
force-pushed
the
zapConfig
branch
from
4030e93 to
4a9ce7c
Compare
abhinavdangeti
left a comment
abhinavdangeti
left a comment
There was a problem hiding this comment.
It seems this commit makes it clear that you'll need to introduce new APIs (blevesearch/zapx#256) to all versions of zapx.
index/scorch/merge.go
Outdated
| prevBytesReadTotal := cumulateBytesRead(segmentsToMerge) | ||
| newDocNums, _, err := s.segPlugin.Merge(segmentsToMerge, docsToDrop, path, | ||
| cw.cancelCh, s) | ||
| newDocNums, _, err := s.segPlugin.MergeEx(segmentsToMerge, docsToDrop, path, |
There was a problem hiding this comment.
Naming will need updates in parlance with comments on blevesearch/zapx#256.
abhinavdangeti
left a comment
abhinavdangeti
left a comment
There was a problem hiding this comment.
Looks fine, but let's wait to get all the zapx version commits in before we take this.
| // New takes a set of Documents and turns them into a new Segment | ||
| New(results []index.Document) (segment.Segment, uint64, error) | ||
|
|
||
| NewUsing(results []index.Document, config map[string]interface{}) (segment.Segment, uint64, error) |
There was a problem hiding this comment.
Rather than define new functions for the interface, can create a new interface like SegmentPluginWithConfig with the new methods. You don't need to change the existing zap versions, and you can use comma-ok idioms to fallback on the existing implementation.
There was a problem hiding this comment.
I think having a new interface with just the *Using APIs would probably force us to maintain a separate plugin and possibly a new registry for the same. Also, the config is supposed to be passed around and useful for the existing paths in scorch where the zap APIs are called - so we could've just updated existing APIs that were there but just to keep the backward compatibility for users of the zap package independently I don't suppose we could do that. The segment plugin definitely needs to be refactor'd though since its messy, and we can talk about it as part of a separate github issue or so?
5 participants
segmentConfigthat can be used to pass index specific config values for the zap level. This would help in the future to perform any index specific tuning at the zap level.segmentConfigis something that's constructed by parsing thescorch.configand persisted in the index_meta.json file.