Skip to content

Conversation

@benclive
Copy link
Contributor

What this PR does / why we need it:
Refactor index calculation code to split the actual work into separate steps.

It works like this:

  1. Each "processLogsSection" call will get a new instance of step objects. They don't hold much state so this should be fine.
  2. Before reading any logs from the section, Prepare is called for each step with the stats/metadata.
  3. The main calculation code handles reading the logs from the object and calls ProcessBatch for each step.
  4. Finally, Flush is called on all the steps so they can write their results to the builder or do any final calculations.

A calculation context is passed to each step so they can extract the tenant ID or any other information they need.

I'm hoping this will keep things relatively tidy as more indexing stategies are introduced over time. As a plus, it also simplifies error handling around the mutex. There is no change to functionality: it is a pure refactor.

@benclive benclive requested a review from a team as a code owner November 24, 2025 12:24
objectPath: objectPath,
sectionIdx: sectionIdx,
streamIDLookup: streamIDLookup,
builderMtx: c.builderMtx,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: sharing mutex pointers can be a source of bugs because the spread of where logic is defined gets wider and it can be harder to keep track of making sure the mutex is being used properly in all places.

I'd recommend considering changing the semantics such that:

  • The mutex is locked whenever we pass the logsCalculationContext to ProcessBatch/Flush
  • We clarify in the interface API that implementations must not retain the builder after the call returns

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I've made the changes you suggested and locked the mutex before calling the step implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants