refactor(fracmanager): using fifo queues of fractions #268

eguguchkin · 2025-11-18T15:47:13Z

Fixes #156

I have read and followed all requirements in CONTRIBUTING.md;
I used LLM/AI assistance to make this pull request;

codecov-commenter · 2025-11-19T08:46:34Z

Codecov Report

❌ Patch coverage is 76.96160% with 138 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.73%. Comparing base (627de89) to head (2e2c673).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
fracmanager/fraction_registry.go	75.25%	39 Missing and 9 partials ⚠️
fracmanager/lifecycle_manager.go	66.36%	25 Missing and 12 partials ⚠️
fracmanager/proxy_frac.go	69.69%	18 Missing and 2 partials ⚠️
fracmanager/fracmanager.go	85.10%	11 Missing and 3 partials ⚠️
frac/active.go	58.82%	4 Missing and 3 partials ⚠️
frac/remote.go	83.33%	3 Missing and 2 partials ⚠️
frac/sealed.go	80.00%	2 Missing and 2 partials ⚠️
fracmanager/loader.go	50.00%	2 Missing ⚠️
cmd/seq-db/seq-db.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
+ Coverage   71.29%   71.73%   +0.44%     
==========================================
  Files         200      202       +2     
  Lines       14557    14516      -41     
==========================================
+ Hits        10378    10413      +35     
+ Misses       3450     3366      -84     
- Partials      729      737       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

forshev · 2025-11-19T11:33:14Z

fracmanager/fracmanager.go

-	fm.fracMu.Unlock()
+		logger.Info("stats loop is started")
+		// Run stats collection every 10 seconds
+		util.RunEvery(ctx.Done(), time.Second*10, func() {


nit: maybe make interval configurable?

forshev · 2025-11-19T11:34:07Z

fracmanager/fraction_registry.go

+
+	stats       registryStats // Size statistics for monitoring
+	oldestTotal uint64        // Creation time of oldest fraction
+	oldestLocal uint64        // Creation time of oldest fraction


// Creation time of oldest fraction -> // Creation time of oldest local fraction?

forshev · 2025-11-19T11:34:25Z

fracmanager/fraction_registry.go

+	remotes    []*remoteProxy // Offloaded fractions (can be thousands)
+
+	stats       registryStats // Size statistics for monitoring
+	oldestTotal uint64        // Creation time of oldest fraction


// Creation time of oldest fraction -> // Creation time of oldest fraction including remote?

forshev · 2025-11-19T11:35:28Z

fracmanager/lifecycle_manager.go

+		go func() {
+			defer wg.Done()
+
+			remote, _ := lc.TryOffload(ctx, sealed.instance)


should we ignore error here?

cheb0 · 2025-12-11T08:16:37Z

cmd/seq-db/seq-db.go

 			MaintenanceDelay:  0,
 			CacheGCDelay:      0,
 			CacheCleanupDelay: 0,
+			MinSealFracSize:   uint64(cfg.Storage.TotalSize) * consts.DefaultMinSealPercent / 100,


nit: I don't get it. It's min seal size for a single frac? Or all fracs? And it's default equal to 20 percent from total size? If total size is 100 gig, then MinSealFracSize is 20 gig. Am I missing something?

cheb0 · 2025-12-11T08:32:03Z

fracmanager/proxy_frac.go

+func (p *activeProxy) Finalize() error {
+	p.mu.Lock()
+	if p.finalized {
+		p.mu.Unlock()


nit: can we use defer here?

cheb0 · 2025-12-11T08:35:49Z

fracmanager/fraction_registry.go

+
+	// Lifecycle queues (FIFO order, oldest at lower indexes)
+	sealing    []*activeProxy // Fractions being sealed (0-5 typical)
+	locals     []*sealedProxy // Local sealed fractions (can be thousands)


nit: is local is a common name for us? seems like sealed is a better name, because local vs remote is harder to understand. I'd expect everything which is not remote to be local by default. But I also understand that sealed clashes with sealing which makes a code harder to read.

cheb0 · 2025-12-11T08:49:08Z

fracmanager/fraction_registry.go

+		})
+	}
+
+	// Init oldest local value


nit: a comment doesn't help in understanding a code

cheb0 · 2025-12-11T08:50:40Z

fracmanager/fraction_registry.go

+		return nil, errors.New("active fraction must be specified")
+	}
+
+	// Set current active fraction


nit: a comments states that we are settings an active fraction, but we actually create a new instance of frac registry

cheb0 · 2025-12-11T09:13:11Z

fracmanager/fracmanager.go

-	}
-	return local
-}
+		// Stop indexer


nit: same thing. IMO unneeded comment

cheb0 · 2025-12-11T09:15:26Z

fracmanager/fraction_registry.go

+// Ensures correct state transitions while maintaining chronological order.
+// The entire structure is thread-safe due to internal synchronization.
+// Lifecycle: Created once, persists through application lifetime.
+type fractionRegistry struct {


nit: looks like this test has to have some kind of concurrency test. Maybe in a different PR

I'd have something which spams bulk concurrently with offloading enabled and with tiny frac size so that we constantly offload and seal. And checking if we see all data as expected at the same time.

cheb0 · 2025-12-11T09:18:21Z

fracmanager/lifecycle_manager.go

+
+// Seal converts an active fraction to sealed state
+// Freezes writes, waits for pending operations, then seals the fraction.
+func (lc *lifecycleManager) Seal(active *activeProxy) error {


nit: once again trying to figure out the naming rule. Can it be named seal here? it's called only from this struct

cheb0 · 2025-12-11T09:50:50Z

fracmanager/fraction_registry.go

+	oldestTotal uint64        // Creation time of oldest fraction
+	oldestLocal uint64        // Creation time of oldest fraction
+
+	muAll  sync.RWMutex    // Mutex specifically for all fractions list


nit: This comment states Mutex specifically for all fractions list, however r.active and r.oldestTotal are also protected by this lock.

eguguchkin requested review from dkharms and forshev November 18, 2025 15:47

eguguchkin added the epic/fracmanager-v2 label Nov 18, 2025

eguguchkin force-pushed the 156-new-lifecycle-manager branch from 3e082e3 to e956ab5 Compare November 18, 2025 16:12

refactor(fracmanager): using fifo queues of fractions

245b875

eguguchkin force-pushed the 156-new-lifecycle-manager branch from e956ab5 to 245b875 Compare November 19, 2025 08:42

fix: return data provider release

2e2c673

forshev approved these changes Nov 26, 2025

View reviewed changes

cheb0 approved these changes Dec 11, 2025

View reviewed changes

refactor(fracmanager): using fifo queues of fractions #268

Are you sure you want to change the base?

refactor(fracmanager): using fifo queues of fractions #268

Uh oh!

Conversation

eguguchkin commented Nov 18, 2025

Uh oh!

codecov-commenter commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Nov 19, 2025 •

edited

Loading