Skip to content

Commit 925e0fb

Browse files
committed
docs: improve code documentation and comments
Add detailed documentation for versioning package: 1. Service methods: - CheckExpected: optimistic locking explanation - repairIntent: crash recovery documentation - Why these mechanisms matter - How to apply them correctly 2. Component methods: - Start: repair and bootstrap documentation - repairOpenIntents: explain crash recovery 3. ResourceStoreAdapter: - Clarify Resource <-> Store model mapping - Document error handling strategy 4. Types: - Intent status transitions - Version lifecycle - Meta pointer semantics All changes are documentation only - no functional changes. Code formatted with go fmt.
1 parent 9d78708 commit 925e0fb

5 files changed

Lines changed: 102 additions & 26 deletions

File tree

pkg/core/versioning/component.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,10 +133,17 @@ func (c *component) Start(rt runtime.Runtime, stop <-chan struct{}) error {
133133
return err
134134
}
135135
rm := rmComp.(manager.ResourceManagerComponent).ResourceManager()
136+
// Repair open intents left by crashes or failed mutations.
137+
// Why: If admin crashed after creating an intent but before the subscriber
138+
// committed it, the intent stays PENDING forever and blocks future writes.
139+
// Repair attempts to commit intents whose desired state matches actual state.
136140
if err := c.repairOpenIntents(rm); err != nil {
137141
return err
138142
}
139-
// Bootstrap: record initial version for all existing rules
143+
// Bootstrap: record initial version for all existing rules.
144+
// Why: Versioning was just enabled or this is the first startup. Existing rules
145+
// have no version history. Recording a BOOTSTRAP version establishes a baseline
146+
// so future mutations have a proper "before" state to diff against.
140147
for _, kind := range governor.RuleResourceKinds.Values() {
141148
// Get the store for this kind and list all resources
142149
rs, err := rm.GetStore(kind)

pkg/core/versioning/resource_store_adapter.go

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,22 @@ import (
3434

3535
var _ Store = &ResourceStoreAdapter{}
3636

37-
// ResourceStoreAdapter adapts the resource store to implement versioning.Store
37+
// ResourceStoreAdapter adapts the resource store to implement versioning.Store.
38+
//
39+
// Why this adapter exists:
40+
// - Dubbo-admin uses a unified resource store for all Kubernetes-style resources
41+
// - Version/Intent/Meta are stored as RuleVersion/RuleIntent resources in the same store
42+
// - This adapter translates Store interface calls into resource CRUD operations
43+
//
44+
// Architecture trade-offs:
45+
// - (+) No separate database schema or connection pool needed
46+
// - (+) Versions/Intents benefit from existing store features (indexing, caching)
47+
// - (-) Some operations require full resource scans (e.g., GetVersionByID)
48+
// - (-) Tied to the resource store lifecycle; cannot be used standalone
49+
//
50+
// Limitations:
51+
// - GetVersionByID: Not implemented (would require scanning all RuleVersions)
52+
// - Query performance depends on index efficiency (ByParentRuleIndexName)
3853
type ResourceStoreAdapter struct {
3954
store store.ResourceStore
4055
}
@@ -98,11 +113,12 @@ func (a *ResourceStoreAdapter) ListVersions(kind coremodel.ResourceKind, resourc
98113
}
99114

100115
func (a *ResourceStoreAdapter) InsertVersion(req InsertRequest, maxVersions int64) (*Version, error) {
101-
// Allocate new version ID (Phase 3 will add proper ID allocation)
102-
// For now, use timestamp-based ID
116+
// ID allocation: use timestamp in milliseconds as a monotonic ID
117+
// Why timestamp: avoids need for separate sequence table; sufficient for version ordering
103118
id := time.Now().UnixNano() / 1000000 // milliseconds
104119

105-
// Allocate version number (count existing versions + 1)
120+
// Allocate version number by counting existing versions
121+
// VersionNo is 1-based and increments with each mutation
106122
versions, err := a.ListVersions(req.RuleKind, req.ResourceKey)
107123
if err != nil {
108124
return nil, err
@@ -142,7 +158,8 @@ func (a *ResourceStoreAdapter) InsertVersion(req InsertRequest, maxVersions int6
142158
return nil, err
143159
}
144160

145-
// Trim old versions
161+
// Trim old versions to enforce maxVersions limit
162+
// Oldest versions are deleted first to keep storage bounded
146163
if maxVersions > 0 {
147164
if err := a.TrimVersions(req.RuleKind, req.ResourceKey, maxVersions); err != nil {
148165
return nil, err

pkg/core/versioning/service.go

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,14 +110,27 @@ func (s *Service) Diff(kind coremodel.ResourceKind, mesh, ruleName string, id in
110110
}, nil
111111
}
112112

113+
// CheckExpected validates that the current version matches the expected version ID.
114+
// This implements optimistic locking for rule mutations.
115+
//
116+
// Why this check matters:
117+
// - Prevents lost updates when two users edit the same rule concurrently
118+
// - Forces clients to acknowledge the current state before mutating
119+
// - Returns IntentPendingError if another mutation is in progress
120+
//
121+
// How to apply:
122+
// - UI should pass the version ID it's editing as ExpectedVersionID
123+
// - If the check fails, client must refresh and retry
113124
func (s *Service) CheckExpected(kind coremodel.ResourceKind, mesh, ruleName string, expected *int64) error {
114125
if err := s.ensureEnabled(); err != nil {
115126
return nil
116127
}
117128
resourceKey := coremodel.BuildResourceKey(mesh, ruleName)
118-
// The expected-version guard first checks for open intents so a second
119-
// writer in the same rule-lock window gets a deterministic 409 before the
120-
// meta current-version pointer has a chance to lag behind the first write.
129+
// Check for open intents first before checking version mismatch.
130+
// Why: If Writer A created an intent at T1, and Writer B checks expected
131+
// version at T2 (before A's subscriber commits), the meta pointer still
132+
// reflects the old version. Without this guard, B would get VersionConflict
133+
// instead of IntentPending, masking the real issue (concurrent write).
121134
intent, err := s.store.OpenIntent(kind, resourceKey)
122135
if err != nil {
123136
return err
@@ -210,6 +223,19 @@ func (s *Service) GetVersion(kind coremodel.ResourceKind, resourceKey string, id
210223
return s.store.GetVersion(kind, resourceKey, id)
211224
}
212225

226+
// repairIntent attempts to resolve a stale or stuck intent.
227+
// Called during startup to recover from crashes, and before mutations to clear pending state.
228+
//
229+
// Why repair is needed:
230+
// - If admin crashes after creating an intent but before the subscriber commits,
231+
// the intent stays PENDING forever, blocking future writes
232+
// - If the actual resource state matches the intent's desired state, we can
233+
// safely commit the intent retroactively
234+
//
235+
// How to apply:
236+
// - Repair runs automatically at startup (component.Start)
237+
// - Also runs before each mutation (prepareRuleMutation) to clear stale intents
238+
// - Returns IntentPendingError if the intent genuinely conflicts with current state
213239
func (s *Service) repairIntent(intent *Intent, current coremodel.Resource, deleted bool) (*Version, error) {
214240
if intent == nil {
215241
return nil, nil
@@ -226,6 +252,7 @@ func (s *Service) repairIntent(intent *Intent, current coremodel.Resource, delet
226252
if intent.Status != IntentStatusPending && intent.Status != IntentStatusApplied {
227253
return nil, ErrVersionIntentNotOpen
228254
}
255+
// If intent was APPLIED or resource state matches intent, commit it
229256
if intent.Status == IntentStatusApplied || IntentMatchesResource(intent, current, deleted) {
230257
if intent.Status == IntentStatusPending {
231258
if err := s.store.MarkIntentApplied(intent.ID); err != nil {
@@ -234,6 +261,7 @@ func (s *Service) repairIntent(intent *Intent, current coremodel.Resource, delet
234261
}
235262
return s.store.CommitIntent(intent.ID, s.maxVersions)
236263
}
264+
// Resource state diverged from intent; cannot auto-repair
237265
return nil, &IntentPendingError{IntentID: intent.ID}
238266
}
239267

@@ -274,6 +302,8 @@ func buildMutationInsertRequest(res coremodel.Resource, op Operation, source Sou
274302
}, nil
275303
}
276304

305+
// IntentMatchesResource checks if the intent's desired state matches actual resource state.
306+
// Used by repair logic to decide if a stale intent can be safely committed.
277307
func IntentMatchesResource(intent *Intent, current coremodel.Resource, deleted bool) bool {
278308
if intent == nil {
279309
return false

pkg/core/versioning/subscriber.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,10 @@ func (s *Subscriber) record(event events.Event) error {
109109
resourceKey := res.ResourceKey()
110110
ruleName := res.ResourceMeta().Name
111111

112-
// Try to commit matching intent first
112+
// Try to commit matching intent first before creating a new UPSTREAM version.
113+
// Why: Admin UI creates an Intent, then applies the mutation. When the subscriber
114+
// sees the resource change, it should commit that Intent rather than creating
115+
// a redundant UPSTREAM version with the same content hash.
113116
committed, err := s.tryCommitMatchingIntent(ruleKind, resourceKey, hash)
114117
if err != nil {
115118
return err
@@ -137,8 +140,10 @@ func (s *Subscriber) record(event events.Event) error {
137140
return fmt.Errorf("failed to get next version number: %w", err)
138141
}
139142

140-
// Check for duplicate hash to avoid redundant versions
141-
// Skip dedup check for DELETE operations (they use a fixed hash and should always be recorded)
143+
// Deduplication: skip creating a version if the content hash matches the latest.
144+
// Why: Upstream changes may fire multiple events with identical content (registry
145+
// restarts, re-registrations). Recording every duplicate wastes storage.
146+
// Skip dedup for DELETE operations: they use a fixed hash and should always be recorded.
142147
if op != OperationDelete {
143148
if exists, err := s.checkDuplicateHash(ruleKind, mesh, ruleName, hash); err != nil {
144149
return fmt.Errorf("failed to check duplicate hash: %w", err)

pkg/core/versioning/types.go

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,15 @@ import (
2424
coremodel "github.com/apache/dubbo-admin/pkg/core/resource/model"
2525
)
2626

27+
// Source identifies where a rule mutation originated.
28+
// Used for auditing and distinguishing user-initiated changes from system-generated ones.
2729
type Source string
2830

2931
const (
30-
SourceAdmin Source = "ADMIN"
31-
SourceUpstream Source = "UPSTREAM"
32-
SourceRollback Source = "ROLLBACK"
33-
SourceBootstrap Source = "BOOTSTRAP"
32+
SourceAdmin Source = "ADMIN" // User edit via Admin UI/API
33+
SourceUpstream Source = "UPSTREAM" // Registry change detected by subscriber
34+
SourceRollback Source = "ROLLBACK" // Rollback to a previous version
35+
SourceBootstrap Source = "BOOTSTRAP" // Initial version recorded at startup
3436
)
3537

3638
type Operation string
@@ -41,26 +43,32 @@ const (
4143
OperationDelete Operation = "DELETE"
4244
)
4345

46+
// IntentStatus tracks the lifecycle of a mutation intent.
47+
// Intent workflow: PENDING (created) → APPLIED (mutation succeeded) → COMMITTED (version recorded)
48+
// Or: PENDING → FAILED (mutation failed or conflicted)
4449
type IntentStatus string
4550

4651
const (
47-
IntentStatusPending IntentStatus = "PENDING"
48-
IntentStatusApplied IntentStatus = "APPLIED"
49-
IntentStatusCommitted IntentStatus = "COMMITTED"
50-
IntentStatusFailed IntentStatus = "FAILED"
52+
IntentStatusPending IntentStatus = "PENDING" // Intent created, mutation not yet applied
53+
IntentStatusApplied IntentStatus = "APPLIED" // Mutation applied to resource store, awaiting commit
54+
IntentStatusCommitted IntentStatus = "COMMITTED" // Version successfully recorded, intent closed
55+
IntentStatusFailed IntentStatus = "FAILED" // Mutation failed or was rejected
5156
)
5257

5358
var (
5459
ErrFeatureDisabled = errors.New("rule versioning is disabled")
55-
ErrVersionConflict = errors.New("rule version conflict")
60+
ErrVersionConflict = errors.New("rule version conflict") // ExpectedVersionID mismatch
5661
ErrVersionNotFound = errors.New("rule version not found")
5762
ErrVersionIntentNotFound = errors.New("rule version intent not found")
58-
ErrVersionIntentNotOpen = errors.New("rule version intent is not open")
59-
ErrVersionIntentPending = errors.New("rule version intent is pending")
63+
ErrVersionIntentNotOpen = errors.New("rule version intent is not open") // Intent already committed or failed
64+
ErrVersionIntentPending = errors.New("rule version intent is pending") // Another mutation in progress
6065
ErrRollbackToDelete = errors.New("cannot roll back to a deleted rule version")
6166
ErrRollbackToCurrent = errors.New("cannot roll back to a version identical to current")
6267
)
6368

69+
// Version represents an immutable snapshot of a rule's spec at a point in time.
70+
// Versions are append-only; each mutation creates a new Version record.
71+
// The IsCurrent field is computed at query time by comparing with Meta.CurrentVersion.
6472
type Version struct {
6573
ID int64 `json:"id" gorm:"primaryKey;autoIncrement"`
6674
RuleKind coremodel.ResourceKind `json:"ruleKind" gorm:"type:varchar(64);not null;index:idx_rk_key_created,priority:1;uniqueIndex:uk_rk_key_ver,priority:1;index:idx_rk_hash,priority:1"`
@@ -83,6 +91,9 @@ func (Version) TableName() string {
8391
return "rule_version"
8492
}
8593

94+
// Meta tracks the current version and sequence number for a rule.
95+
// Updated atomically when a new version is committed.
96+
// CurrentVersion may be nil if the rule was deleted.
8697
type Meta struct {
8798
RuleKind coremodel.ResourceKind `json:"ruleKind" gorm:"type:varchar(64);primaryKey"`
8899
ResourceKey string `json:"resourceKey" gorm:"type:varchar(512);primaryKey"`
@@ -95,6 +106,12 @@ func (Meta) TableName() string {
95106
return "rule_version_meta"
96107
}
97108

109+
// Intent represents a pending mutation to a rule, used to coordinate async writes.
110+
// Why Intents exist:
111+
// - Admin UI mutations are not immediately applied; they create an Intent first
112+
// - The Intent enforces optimistic locking via ExpectedVersionID
113+
// - When the mutation completes, the subscriber commits the Intent to a Version
114+
// - This prevents race conditions when multiple writers target the same rule
98115
type Intent struct {
99116
ID int64 `json:"id" gorm:"primaryKey;autoIncrement"`
100117
RuleKind coremodel.ResourceKind `json:"ruleKind" gorm:"type:varchar(64);not null;index:idx_intent_rule_status,priority:1;index:idx_intent_hash,priority:1"`
@@ -107,10 +124,10 @@ type Intent struct {
107124
Operation Operation `json:"operation" gorm:"type:varchar(16);not null"`
108125
Author string `json:"author" gorm:"type:varchar(128);not null"`
109126
Reason string `json:"reason,omitempty" gorm:"type:varchar(1024)"`
110-
RolledBackFromID *int64 `json:"rolledBackFromId,omitempty"`
111-
ExpectedVersionID *int64 `json:"expectedVersionId,omitempty"`
127+
RolledBackFromID *int64 `json:"rolledBackFromId,omitempty"` // Points to the Version being rolled back to
128+
ExpectedVersionID *int64 `json:"expectedVersionId,omitempty"` // Optimistic lock: mutation only proceeds if current version matches
112129
Status IntentStatus `json:"status" gorm:"type:varchar(16);not null;index:idx_intent_rule_status,priority:3"`
113-
VersionID *int64 `json:"versionId,omitempty"`
130+
VersionID *int64 `json:"versionId,omitempty"` // Set when committed; points to the created Version
114131
LastError string `json:"lastError,omitempty" gorm:"type:varchar(1024)"`
115132
CreatedAt time.Time `json:"createdAt" gorm:"not null"`
116133
UpdatedAt time.Time `json:"updatedAt" gorm:"not null"`

0 commit comments

Comments
 (0)