Common cycle manager #3025

aliszka · 2023-05-15T21:43:42Z

What's being changed:

Addresses #2983
Instead creating multiple cycle managers, separate for each bucket, geo prop etc, common cycle managers are created for different usecases:

memtable flush (one per store)
compaction (one per store)
tombstone cleanup (one per vector index, if created)
commit log maintenance (one per vector index, if created)
tombstone cleanup for geo props (one per shard, if any geo prop is indexed)
commit log maintenance for geo props (one per shard, if any geo prop is indexed)

Review checklist

Documentation has been updated, if necessary. Link to changed documentation:
Chaos pipeline run or not necessary. Link to pipeline: [TEST] common cycle manager weaviate-chaos-engineering#75
All new code is covered by tests where it is reasonable.
Performance tests have been run or not necessary.

redouan-rhazouani · 2023-05-17T17:40:05Z

adapters/repos/db/vector/hnsw/maintenance_cycles.go

+		c.commitLogMaintenance = cyclemanager.NewMulti(commitlogMaintenanceTicker)
+		c.tombstoneCleanup = cyclemanager.NewMulti(tombstoneCleanupTicker)
+
+		c.commitLogMaintenance.Start()


c.commitLogMaintenance or c.tombstoneCleanup one of them could be nil
I think we need to check for nil pointer before calling start

cyclemanager.NewMulti() method always returns instance. so at that point both of cycle managers are inited and safe to use

redouan-rhazouani · 2023-05-17T17:41:38Z

adapters/repos/db/vector/hnsw/maintenance_cycles.go

+		tombstoneCleanupStop <- nil
+	}()
+
+	commitlogMaintenanceStopErr := <-commitlogMaintenanceStop


the main goroutine blocks anyway. I would rather use a wait group instead of using two channels

Right, refactored to use WaitGroup. Also extracted to separate method to be reused in Shutdown method

redouan-rhazouani · 2023-05-17T17:42:20Z

adapters/repos/db/vector/hnsw/maintenance_cycles.go

+	}
+	c.lock.Unlock()
+
+	c.commitLogMaintenance.Start()


Same as before we need to check for nit pointers

as explained above. method returns earlier if instance is not inited.

redouan-rhazouani · 2023-05-17T17:43:11Z

adapters/repos/db/vector/hnsw/maintenance_cycles.go

+	commitlogMaintenanceStop := make(chan error)
+	tombstoneCleanupStop := make(chan error)
+	go func() {
+		if err := c.commitLogMaintenance.StopAndWait(ctx); err != nil {


check nil for nil pointer

Check whether commitLogMaintenance and tombstoneCleanup are nils is done at the beginning of the method. If so, method returns earlier.

I've extracted the check to separate method, as it was used couple of times already, and changed condition to consider instance inited if both cycle managers are created, not just one.
(change of condition was purely cosmetic, cause both cycle managers are created at the same time. So either both or none will be created)

redouan-rhazouani · 2023-05-17T17:43:26Z

adapters/repos/db/vector/hnsw/maintenance_cycles.go

+		commitlogMaintenanceStop <- nil
+	}()
+	go func() {
+		if err := c.tombstoneCleanup.StopAndWait(ctx); err != nil {


here as well

as explained above. method returns earlier if instance is not inited.

redouan-rhazouani · 2023-05-17T17:43:49Z

adapters/repos/db/vector/hnsw/maintenance_cycles.go

+
+	eg := errgroup.Group{}
+	eg.Go(func() error {
+		if err := c.commitLogMaintenance.StopAndWait(ctx); err != nil {


check nil for nil pointer

as explained above. method returns earlier if instance is not inited.

redouan-rhazouani · 2023-05-17T17:44:51Z

entities/cyclemanager/callbacks.go

+		counter:     0,
+		states:      map[uint32]*callbackState{},
+		keys:        []uint32{},
+		canceledCtx: canceledCtx,


It is not clear to me why are we returning cancelled context canceledCtx (see line 41)

I used contexts to track if callback is being currently executed. Active context means callback is running, expired context means callback finished execution or was not yet started.

This particular canceled (expired) context was meant to be used as const context, for all newly added callbacks to mark them as not running.

I've changed the implementation to use nil to mark not yet run callbacks instead of expired ctx.

redouan-rhazouani · 2023-05-17T17:51:17Z

entities/cyclemanager/callbacks.go

+			continue
+		case <-ctx.Done():
+			// in case both context are ready, but incoming ctx was selected
+			// check again running ctx as priority one


I would consider using if runningCtx.Err() != nil instead of the select because it is more simple

Good point. Changed to error check.

antas-marcin · 2023-05-18T09:02:22Z

adapters/repos/db/lsmkv/store_backup_test.go

+	"github.com/weaviate/weaviate/entities/storagestate"
+)
+
+func TestStoreBackup_PauseCompaction(t *testing.T) {


Maybe it would be also useful to include in all tests a scenario where we create n-buckets in store? now we create in each test only 1 bucket.

added tests for 1, 2 and 5 buckets

great! looks good

antas-marcin · 2023-05-18T09:14:01Z

adapters/repos/db/vector/hnsw/startup.go

+	// h.tombstoneCleanupCycle.Start()
+	// h.commitLogMaintenanceCycle.Start()


why is this commented?

good catch. removed

aliszka force-pushed the common_cycle_manager branch from a5872bf to e561dfd Compare May 16, 2023 15:51

aliszka mentioned this pull request May 16, 2023

[TEST] common cycle manager weaviate/weaviate-chaos-engineering#75

Closed

aliszka marked this pull request as ready for review May 17, 2023 06:52

redouan-rhazouani suggested changes May 17, 2023

View reviewed changes

antas-marcin reviewed May 18, 2023

View reviewed changes

aliszka added 8 commits May 18, 2023 13:52

introduces callbacks for cycle manager

cad6e92

replaces cycle manager's cyclefunc dependency with callbacks

56b8292

common compaction cycle manager

f0418ad

common flush cycle manager

8157c68

common commitlog maintenance cycle manager

feae593

common commitlog maintenance cycle manager for geo props

4781e6a

moves cycle managers outside from hnsw, introduces maintenance cycles

674aaa8

addresses code review feedback

54ccdce

aliszka force-pushed the common_cycle_manager branch from 066ddf0 to 54ccdce Compare May 18, 2023 11:53

antas-marcin approved these changes May 18, 2023

View reviewed changes

antas-marcin merged commit fc67581 into stable/v1.19 May 22, 2023
12 checks passed

antas-marcin deleted the common_cycle_manager branch May 22, 2023 10:23

trengrj mentioned this pull request Sep 1, 2023

Only one cycle manager per shard #2983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common cycle manager #3025

Common cycle manager #3025

aliszka commented May 15, 2023 •

edited

redouan-rhazouani May 17, 2023

aliszka May 18, 2023

redouan-rhazouani May 17, 2023

aliszka May 18, 2023

redouan-rhazouani May 17, 2023

aliszka May 18, 2023

redouan-rhazouani May 17, 2023

aliszka May 18, 2023

redouan-rhazouani May 17, 2023

aliszka May 18, 2023

redouan-rhazouani May 17, 2023

aliszka May 18, 2023

redouan-rhazouani May 17, 2023

aliszka May 18, 2023

redouan-rhazouani May 17, 2023

aliszka May 18, 2023

antas-marcin May 18, 2023

aliszka May 18, 2023

antas-marcin May 18, 2023

antas-marcin May 18, 2023

aliszka May 18, 2023

		// h.tombstoneCleanupCycle.Start()
		// h.commitLogMaintenanceCycle.Start()

Common cycle manager #3025

Common cycle manager #3025

Conversation

aliszka commented May 15, 2023 • edited

What's being changed:

Review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliszka commented May 15, 2023 •

edited