Compactor Memory Improvements/Bug fix #1130
Conversation
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
joe-elliott
requested review from
annanay25,
dgzlopes,
kvrhdn,
mapno and
mdisibio
as code owners
Signed-off-by: Joe Elliott <[email protected]>
joe-elliott
changed the title
Signed-off-by: Joe Elliott <[email protected]>
| return | ||
| } | ||
| for _, b := range lowestBookmarks { | ||
| b.clear() |
There was a problem hiding this comment.
I don't understand the logic here. Why are we appending to lowestBookmarks if we clear them before using them in the log? Could we use a counter instead?
There was a problem hiding this comment.
lowestBookmarks is a slice that contains bookmark structs that "point to" the next lowest object for compaction. Clearing them just tells them to move past their current object. The reason we do it here is b/c if the condition below hits and we continue then we need to be sure that we've moved these bookmarks to the next thing or we will just get in an infinite loop.
Honestly unsure about the quality of that log. Might make more sense to just log len(lowestBookmarks). The only thing I really wanted was info about why the condition evaluated to true.
There was a problem hiding this comment.
The log message previously printed lowestBookmark, which mainly was to distinguish between nil or not (when not nil it just prints unsupported object type). It would be similar here, printing nil/empty slice/nil/non-nil entries quite messily.... but quite frankly since we haven't determined root cause for the empty objects yet, I am ok with that and prefer it over a simple len().
What this PR does:
b.clear()on bookmarks that did not represent the lowest id.TestSameIDCompactiondoes show errors. This probably resulted in some lost spans.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]