Skip to content

refactor: Remove timeSrc from QueueStore#7735

Merged
natemort merged 1 commit intocadence-workflow:masterfrom
ribaraka:mv-timeSrc-to-persistManager
Feb 23, 2026
Merged

refactor: Remove timeSrc from QueueStore#7735
natemort merged 1 commit intocadence-workflow:masterfrom
ribaraka:mv-timeSrc-to-persistManager

Conversation

@ribaraka
Copy link
Contributor

What changed?
Removed timeSrc from QueueStore. Added lazy metadata initializaion.

Why?
#6610
#6646 (comment)
The queue metadata was initialized too early, during newNoSQLQueueStore construction, before the manager layer existed, so the store had to generate time itself. This violated the goal of having all time generation happen at the persistence manager layer.
The fix defer metadata creation to the first operation that naturally carries a timestamp from the manager, instead of initializing it at construction time.

How did you test it?
unit test: go test ./common/persistence/nosql -v
integrated test: Started local server and run make test_e2e

Potential risks
No risk changes

Release notes

Documentation Changes


Reviewer Validation

PR Description Quality (check these before reviewing code):

  • "What changed" provides a clear 1-2 line summary
    • Project Issue is linked
  • "Why" explains the full motivation with sufficient context
  • Testing is documented:
    • Unit test commands are included (with exact go test invocation)
    • Integration test setup/commands included (if integration tests were run)
    • Canary testing details included (if canary was mentioned)
  • Potential risks section is thoughtfully filled out (or legitimately N/A)
  • Release notes included if this completes a user-facing feature
  • Documentation needs are addressed (or noted if uncertain)

…ion in queue write/ack paths via ensureQueueMetadata

Signed-off-by: Stanislav Bychkov <stanislb@netapp.com>
if queueMetadata == nil {
return nil, insertErr
}
} else {
Copy link

Choose a reason for hiding this comment

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

💡 Performance: Redundant re-read after successful metadata insert

In ensureQueueMetadata, after a successful insertInitialQueueMetadataRecord (line 85-94), the code re-reads the metadata from the database with getQueueMetadata. Since we just successfully inserted it, the metadata state is known — we could construct the QueueMetadataRow directly from the values we just inserted (version 0, empty ClusterAckLevels, the provided currentTimestamp) instead of making another round-trip to the database.

This is a very minor concern since it only happens once per queue lifetime (on first initialization), but it does add an unnecessary database read on the happy path.

Suggested fix:

	} else {
			queueMetadata = &nosqlplugin.QueueMetadataRow{
				QueueType:        queueType,
				Version:          0,
				ClusterAckLevels: map[string]int64{},
				CurrentTimeStamp: currentTimestamp,
			}
		}

Was this helpful? React with 👍 / 👎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to re-read after insert so to return the authentic row, not a hand-made struct. It also guards against false positive cases in insertInitialQueueMetadataRecord.
And it has been said: cost is negligible since this runs only once during metadata initialization.

@gitar-bot
Copy link

gitar-bot bot commented Feb 23, 2026

Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Clean refactoring that correctly removes timeSrc from the store layer and introduces well-structured lazy metadata initialization with proper concurrency handling. One minor suggestion to avoid a redundant database read after successful insert.

💡 Performance: Redundant re-read after successful metadata insert

📄 common/persistence/nosql/nosql_queue_store.go:85

In ensureQueueMetadata, after a successful insertInitialQueueMetadataRecord (line 85-94), the code re-reads the metadata from the database with getQueueMetadata. Since we just successfully inserted it, the metadata state is known — we could construct the QueueMetadataRow directly from the values we just inserted (version 0, empty ClusterAckLevels, the provided currentTimestamp) instead of making another round-trip to the database.

This is a very minor concern since it only happens once per queue lifetime (on first initialization), but it does add an unnecessary database read on the happy path.

Suggested fix
	} else {
			queueMetadata = &nosqlplugin.QueueMetadataRow{
				QueueType:        queueType,
				Version:          0,
				ClusterAckLevels: map[string]int64{},
				CurrentTimeStamp: currentTimestamp,
			}
		}
Rules ❌ No requirements met

Repository Rules

PR Description Quality Standards: Move issue link to What changed section, provide reproducible test commands with setup instructions, and expand risk analysis for persistence layer changes

1 rule not applicable. Show all rules by commenting gitar display:verbose.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@natemort natemort merged commit 3c92730 into cadence-workflow:master Feb 23, 2026
42 checks passed
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.

5 participants