Skip to content

refactor(config): use generic cache for SegmentWriter#7316

Open
Mertozturkk wants to merge 3 commits intoJanDeDobbeleer:mainfrom
Mertozturkk:refactor/generic-segment-writer
Open

refactor(config): use generic cache for SegmentWriter#7316
Mertozturkk wants to merge 3 commits intoJanDeDobbeleer:mainfrom
Mertozturkk:refactor/generic-segment-writer

Conversation

@Mertozturkk
Copy link
Copy Markdown

Prerequisites

  • I have read and understood the contributing guide.
  • The commit message follows the conventional commits guidelines.
  • Tests for the changes have been added (for bug fixes / features).
  • Docs have been added/updated (for bug fixes / features).

Description

This is a reworked version of #7308, which was unintentionally closed during a base branch rebase. The changes have been rebased onto the latest main and address all review feedback from the previous PR.

Refactored the segment caching mechanism to use generic types instead of JSON serialization, resolving the TODO in setCache.

Changes:

  • Replaced json.Marshal/json.Unmarshal with cache.Get[SegmentWriter] and cache.Set for direct interface storage
  • Added graceful handling of legacy string cache entries (deletes old keys without error-level logging)
  • Added TestSegmentCache unit test covering both generic cache and legacy data migration
  • Added defer cache.DeleteAll to prevent test state leakage

Copilot AI review requested due to automatic review settings February 17, 2026 07:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors segment caching in config.Segment to store and restore SegmentWriter values directly via the generic cache API (instead of JSON serialization), while attempting to migrate legacy string cache entries.

Changes:

  • Switch segment cache storage from json.Marshal/json.Unmarshal to cache.Set/cache.Get[SegmentWriter].
  • Add legacy-cache detection that deletes string cache entries so they can be regenerated.
  • Add a unit test covering writer restoration and legacy-entry deletion.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/config/segment.go Updates segment cache restore/set logic to use typed cache storage and adds legacy string cleanup.
src/config/segment_cache_test.go Adds a unit test for segment cache restore and legacy key removal.

@Mertozturkk Mertozturkk force-pushed the refactor/generic-segment-writer branch from be3e849 to e04dca6 Compare February 17, 2026 07:21
@JanDeDobbeleer JanDeDobbeleer force-pushed the main branch 2 times, most recently from 6a67260 to 85091a8 Compare February 18, 2026 15:59
Copilot AI review requested due to automatic review settings February 21, 2026 15:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

}

// Never cache pending state to avoid polluting cache with incomplete data
if segment.Pending {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

don't we want to keep this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That check was accidentally removed during the refactor. I've restored it to ensure we don't pollute the cache with incomplete data when a segment is still in a pending state. Re-pushed the fix

@Mertozturkk Mertozturkk force-pushed the refactor/generic-segment-writer branch from 143fef6 to a520b40 Compare March 3, 2026 06:36
Copilot AI review requested due to automatic review settings March 3, 2026 06:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

@Mertozturkk Mertozturkk force-pushed the refactor/generic-segment-writer branch from a520b40 to 59f231b Compare March 3, 2026 06:51
Copilot AI review requested due to automatic review settings March 5, 2026 07:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/config/segment.go:377

  • Segment cache entries now store gob-encoded []byte under the existing "segment_cache_%s" keys. This changes the value type in-place, which can make troubleshooting/downgrades noisier (older versions expecting a JSON string will see a type mismatch/unmarshal failure in debug logs and won’t self-heal). Consider versioning/namespacing the cache key (e.g., "segment_cache_v2_%s") so different on-disk formats can coexist and upgrades/downgrades degrade to a cache miss instead of repeated decode errors.
func (segment *Segment) cacheKeyAndStore() (string, cache.Store) {
	format := "segment_cache_%s"
	switch segment.Cache.Strategy {
	case Session:
		return fmt.Sprintf(format, segment.Name()), cache.Session
	case Device:
		return fmt.Sprintf(format, segment.Name()), cache.Device
	case Folder:
		fallthrough
	default:
		return fmt.Sprintf(format, strings.Join([]string{segment.Name(), segment.folderKey()}, "_")), cache.Device
	}

You can also share your feedback on Copilot code review. Take the survey.

dohzya and others added 3 commits March 15, 2026 10:07
Add iso8601 and iso8601ms styles for execution time display using
ISO 8601 duration format (PT[n]H[n]M[n]S). The iso8601 style rounds
to nearest second, while iso8601ms preserves milliseconds precision.

Co-authored-by: Claude <noreply@anthropic.com>
@JanDeDobbeleer
Copy link
Copy Markdown
Owner

@Mertozturkk this breaks when in streaming mode, didn't look at the details just yet but I can make the prompt fail now. I pushed the udpated branch, maybe I missed something silly.

@JanDeDobbeleer JanDeDobbeleer force-pushed the refactor/generic-segment-writer branch from a97bf15 to b1f5d4f Compare March 15, 2026 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants