Fix infostream_to_segments.py to parse compact segment descriptors#597
Conversation
|
Phew, good catch, thank you @jerinthomas1404 -- this is from recent optimizations to reduce the insane verbosity of That change logs the verbose details for a segment exactly once, when the segment is born, and skips it thereafter. We don't lose any information because those verbose details never change for a segment. The one place we do lose just a bit is when there is a pre-existing index when |
mikemccand
left a comment
There was a problem hiding this comment.
Thank you @jerinthomas1404!
87d6097 to
e927ca0
Compare
|
|
||
|
|
||
| # When diagnostics are unavailable (compact segment descriptors from pre-existing | ||
| # index segments), we assume "merge" as the default source since long-lived |
There was a problem hiding this comment.
Actually, from your response to my first comment on my last review it looks like Lucene should include the full verbose diagnostics of those initial segments? Let's fix the comment -- maybe change to something like "compact segment descriptors because the verbose version was already logged once for this segment" or so?
Or are you seeing pre-existing segments failing to have verbosity in a new IndexWriter?
There was a problem hiding this comment.
@mikemccand , you're correct, I have verified the upstream code and the init verbose log runs unconditionally.
I have rephrased the comment as below
Fallback when parsing a log that doesn't contain the verbose segment details
(e.g. truncated log missing the init/flush/merge birth line). We assume
"merge" since long-lived indices are dominated by merged segments
735d5e4 to
06414b1
Compare
Since apache/lucene#15885, IndexWriter emits verbose segment details (diagnostics, attributes, id) only once at segment creation using write-once semantics. Subsequent references use a compact form like `_4(10.4.0):C8782/109` with no trailing metadata. Two changes: 1. Make the verbose trailer optional in `re_segment_desc` by wrapping it in a non-capturing group `(?:...)?`. Group numbering (1-8) is preserved so all downstream consumers work unchanged. 2. Handle empty `diagnostics` in `parse_seg_details()` by defaulting to `DEFAULT_SEGMENT_SOURCE = ("merge", [])` — long-lived indices are dominated by merged segments, making merge the better heuristic for pre-existing segments whose origin is unknown. Fixes mikemccand#596
06414b1 to
3c83552
Compare
mikemccand
left a comment
There was a problem hiding this comment.
Looks great! Thank you @jerinthomas1404 -- I'll merge.
The parser fails with
RuntimeError: failed to parse the expected N number of sogmentswhen InfoStream logs contain compact segment descriptors like_4(10.4.0):C8782/109instead of the verbose form with the full:[diagnostics={...}]:[attributes={...}] :id=...trailer.Two changes:
re_segment_descby wrapping it in a non-capturing group(?:...)?. Group numbering (1-8) is preserved so all downstream consumers work unchanged.diagnosticsinparse_seg_details()by defaulting tosource = "flush"instead of raising. This is correct for the common case (writer starting from empty index) where the real flush/merge origin is derived fromflush postings as segmentandmerge seg=events independently.Fixes #596