-
Notifications
You must be signed in to change notification settings - Fork 11
chore: smarter loading scheme #696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
01f8114
8f3bce9
14388c6
d156635
e8e5556
2bd44e2
3070be2
fc17063
be1cb2b
22d112e
44f4dc9
c186f4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| package kafka | ||
|
|
||
| import ( | ||
| "context" | ||
| "math" | ||
| "workspace-engine/pkg/db" | ||
|
|
||
| "github.com/charmbracelet/log" | ||
| "github.com/confluentinc/confluent-kafka-go/v2/kafka" | ||
| ) | ||
|
|
@@ -27,3 +31,45 @@ func createConsumer() (*kafka.Consumer, error) { | |
|
|
||
| return c, nil | ||
| } | ||
|
|
||
| func getEarliestOffset(snapshots map[string]*db.WorkspaceSnapshot) int64 { | ||
| beginning := int64(kafka.OffsetBeginning) | ||
| if len(snapshots) == 0 { | ||
| return beginning | ||
| } | ||
|
|
||
| earliestOffset := int64(math.MaxInt64) | ||
| for _, snapshot := range snapshots { | ||
| if snapshot.Offset < earliestOffset { | ||
| earliestOffset = snapshot.Offset | ||
| } | ||
| } | ||
| if earliestOffset == math.MaxInt64 { | ||
| return beginning | ||
| } | ||
| return earliestOffset | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| func setOffsets(ctx context.Context, consumer *kafka.Consumer, partitionWorkspaceMap map[int32][]string) { | ||
| for partition, workspaceIDs := range partitionWorkspaceMap { | ||
| snapshots, err := db.GetLatestWorkspaceSnapshots(ctx, workspaceIDs) | ||
| if err != nil { | ||
| log.Error("Failed to get latest workspace snapshots", "error", err) | ||
| continue | ||
| } | ||
|
|
||
| earliestOffset := getEarliestOffset(snapshots) | ||
| effectiveOffset := earliestOffset | ||
| if effectiveOffset > 0 { | ||
| effectiveOffset = effectiveOffset + 1 | ||
| } | ||
| if err := consumer.Seek(kafka.TopicPartition{ | ||
| Topic: &Topic, | ||
| Partition: partition, | ||
| Offset: kafka.Offset(effectiveOffset), | ||
| }, 0); err != nil { | ||
| log.Error("Failed to seek to earliest offset", "error", err) | ||
| continue | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+57
to
+79
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partition mismatch risk: filter snapshots by current partition before computing earliest offset. A workspace’s latest snapshot might be from a different partition (e.g., after partition-count changes). Using that offset to seek this partition is incorrect. Apply this diff: - snapshots, err := db.GetLatestWorkspaceSnapshots(ctx, workspaceIDs)
+ snapshots, err := db.GetLatestWorkspaceSnapshots(ctx, workspaceIDs)
if err != nil {
log.Error("Failed to get latest workspace snapshots", "error", err)
continue
}
- earliestOffset := getEarliestOffset(snapshots)
+ // Only consider snapshots that belong to this partition.
+ filtered := make(map[string]*db.WorkspaceSnapshot, len(snapshots))
+ for wid, s := range snapshots {
+ if s != nil && s.Partition == partition {
+ filtered[wid] = s
+ }
+ }
+ earliestOffset := getEarliestOffset(filtered)
effectiveOffset := earliestOffset
- if effectiveOffset > 0 {
- effectiveOffset = effectiveOffset + 1
+ if effectiveOffset >= 0 {
+ effectiveOffset++
}
if err := consumer.Seek(kafka.TopicPartition{
Topic: &Topic,
Partition: partition,
Offset: kafka.Offset(effectiveOffset),
}, 0); err != nil {
- log.Error("Failed to seek to earliest offset", "error", err)
+ log.Error("Failed to seek", "partition", partition, "targetOffset", effectiveOffset, "error", err)
continue
}🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,25 +2,30 @@ package kafka | |||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||||||||||||||||||||
| "encoding/json" | ||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||||||||||||||||||||
| "strconv" | ||||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| "workspace-engine/pkg/db" | ||||||||||||||||||||||||||||||||||||||||||||
| "workspace-engine/pkg/events" | ||||||||||||||||||||||||||||||||||||||||||||
| eventHanlder "workspace-engine/pkg/events/handler" | ||||||||||||||||||||||||||||||||||||||||||||
| "workspace-engine/pkg/oapi" | ||||||||||||||||||||||||||||||||||||||||||||
| "workspace-engine/pkg/workspace" | ||||||||||||||||||||||||||||||||||||||||||||
| wskafka "workspace-engine/pkg/workspace/kafka" | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| "github.com/aws/smithy-go/ptr" | ||||||||||||||||||||||||||||||||||||||||||||
| "github.com/charmbracelet/log" | ||||||||||||||||||||||||||||||||||||||||||||
| "github.com/confluentinc/confluent-kafka-go/v2/kafka" | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // Configuration variables loaded from environment | ||||||||||||||||||||||||||||||||||||||||||||
| var ( | ||||||||||||||||||||||||||||||||||||||||||||
| Topic = getEnv("KAFKA_TOPIC", "workspace-events") | ||||||||||||||||||||||||||||||||||||||||||||
| GroupID = getEnv("KAFKA_GROUP_ID", "workspace-engine") | ||||||||||||||||||||||||||||||||||||||||||||
| Brokers = getEnv("KAFKA_BROKERS", "localhost:9092") | ||||||||||||||||||||||||||||||||||||||||||||
| Topic = getEnv("KAFKA_TOPIC", "workspace-events") | ||||||||||||||||||||||||||||||||||||||||||||
| GroupID = getEnv("KAFKA_GROUP_ID", "workspace-engine") | ||||||||||||||||||||||||||||||||||||||||||||
| Brokers = getEnv("KAFKA_BROKERS", "localhost:9092") | ||||||||||||||||||||||||||||||||||||||||||||
| MinSnapshotDistance = getEnvInt("SNAPSHOT_DISTANCE_MINUTES", 60) | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // getEnv retrieves an environment variable or returns a default value | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -32,6 +37,40 @@ func getEnv(varName string, defaultValue string) string { | |||||||||||||||||||||||||||||||||||||||||||
| return v | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // getEnvInt retrieves an integer environment variable or returns a default value | ||||||||||||||||||||||||||||||||||||||||||||
| func getEnvInt(varName string, defaultValue int64) int64 { | ||||||||||||||||||||||||||||||||||||||||||||
| v := os.Getenv(varName) | ||||||||||||||||||||||||||||||||||||||||||||
| if v == "" { | ||||||||||||||||||||||||||||||||||||||||||||
| return defaultValue | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| i, err := strconv.ParseInt(v, 10, 64) | ||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| log.Warn("Failed to parse environment variable as integer, using default", "var", varName, "value", v, "default", defaultValue) | ||||||||||||||||||||||||||||||||||||||||||||
| return defaultValue | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| return i | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| func getLastSnapshot(ctx context.Context, msg *kafka.Message) (*db.WorkspaceSnapshot, error) { | ||||||||||||||||||||||||||||||||||||||||||||
| var rawEvent eventHanlder.RawEvent | ||||||||||||||||||||||||||||||||||||||||||||
| if err := json.Unmarshal(msg.Value, &rawEvent); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| log.Error("Failed to unmarshal event", "error", err, "message", string(msg.Value)) | ||||||||||||||||||||||||||||||||||||||||||||
| return nil, fmt.Errorf("failed to unmarshal event: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return db.GetWorkspaceSnapshot(ctx, rawEvent.WorkspaceID) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+39
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don’t log full payloads on JSON errors. Logging string(msg.Value) risks PII leakage and noisy logs. func getLastSnapshot(ctx context.Context, msg *kafka.Message) (*db.WorkspaceSnapshot, error) {
- var rawEvent eventHanlder.RawEvent
+ var rawEvent eventHanlder.RawEvent
if err := json.Unmarshal(msg.Value, &rawEvent); err != nil {
- log.Error("Failed to unmarshal event", "error", err, "message", string(msg.Value))
+ log.Error("Failed to unmarshal event", "error", err,
+ "topic", *msg.TopicPartition.Topic,
+ "partition", msg.TopicPartition.Partition,
+ "offset", msg.TopicPartition.Offset)
return nil, fmt.Errorf("failed to unmarshal event: %w", err)
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| func getLastWorkspaceOffset(snapshot *db.WorkspaceSnapshot) int64 { | ||||||||||||||||||||||||||||||||||||||||||||
| beginning := int64(kafka.OffsetBeginning) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if snapshot == nil { | ||||||||||||||||||||||||||||||||||||||||||||
| return beginning | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return snapshot.Offset | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+49
to
+57
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard against cross-partition/topology mismatches when using stored offsets. Using a stored offset from a different partition or topology can skip/duplicate processing. -func getLastWorkspaceOffset(snapshot *db.WorkspaceSnapshot) int64 {
+func getLastWorkspaceOffset(snapshot *db.WorkspaceSnapshot, currentPartition int32, currentNumPartitions int32) int64 {
beginning := int64(kafka.OffsetBeginning)
if snapshot == nil {
return beginning
}
- return snapshot.Offset
+ if snapshot.Partition != currentPartition || snapshot.NumPartitions != currentNumPartitions {
+ return beginning
+ }
+ return snapshot.Offset
}
@@
- lastWorkspaceOffset := getLastWorkspaceOffset(lastSnapshot)
+ lastWorkspaceOffset := getLastWorkspaceOffset(
+ lastSnapshot,
+ msg.TopicPartition.Partition,
+ numPartitions,
+ )Also applies to: 175-182 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // RunConsumerWithWorkspaceLoader starts the Kafka consumer with workspace-based offset resume | ||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||
| // Flow: | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -83,11 +122,17 @@ func RunConsumer(ctx context.Context) error { | |||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| log.Info("Partition assignment complete", "assigned", assignedPartitions) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| allWorkspaceIDs, err := wskafka.GetAssignedWorkspaceIDs(ctx, assignedPartitions, numPartitions) | ||||||||||||||||||||||||||||||||||||||||||||
| partitionWorkspaceMap, err := wskafka.GetAssignedWorkspaceIDs(ctx, assignedPartitions, numPartitions) | ||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Errorf("failed to get assigned workspace IDs: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // Flatten the map to get all workspace IDs | ||||||||||||||||||||||||||||||||||||||||||||
| var allWorkspaceIDs []string | ||||||||||||||||||||||||||||||||||||||||||||
| for _, workspaceIDs := range partitionWorkspaceMap { | ||||||||||||||||||||||||||||||||||||||||||||
| allWorkspaceIDs = append(allWorkspaceIDs, workspaceIDs...) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| storage := workspace.NewFileStorage("./state") | ||||||||||||||||||||||||||||||||||||||||||||
| if workspace.IsGCSStorageEnabled() { | ||||||||||||||||||||||||||||||||||||||||||||
| storage, err = workspace.NewGCSStorageClient(ctx) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -118,6 +163,8 @@ func RunConsumer(ctx context.Context) error { | |||||||||||||||||||||||||||||||||||||||||||
| // Start consuming messages | ||||||||||||||||||||||||||||||||||||||||||||
| handler := events.NewEventHandler() | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| setOffsets(ctx, consumer, partitionWorkspaceMap) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| for { | ||||||||||||||||||||||||||||||||||||||||||||
| // Check for cancellation | ||||||||||||||||||||||||||||||||||||||||||||
| select { | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -134,21 +181,46 @@ func RunConsumer(ctx context.Context) error { | |||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| ws, err := handler.ListenAndRoute(ctx, msg) | ||||||||||||||||||||||||||||||||||||||||||||
| lastSnapshot, err := getLastSnapshot(ctx, msg) | ||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| log.Error("Failed to route message", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||
| log.Error("Failed to get last snapshot", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| snapshot := &db.WorkspaceSnapshot{ | ||||||||||||||||||||||||||||||||||||||||||||
| Path: fmt.Sprintf("%s.gob", ws.ID), | ||||||||||||||||||||||||||||||||||||||||||||
| Timestamp: msg.Timestamp, | ||||||||||||||||||||||||||||||||||||||||||||
| Partition: int32(msg.TopicPartition.Partition), | ||||||||||||||||||||||||||||||||||||||||||||
| NumPartitions: numPartitions, | ||||||||||||||||||||||||||||||||||||||||||||
| messageOffset := int64(msg.TopicPartition.Offset) | ||||||||||||||||||||||||||||||||||||||||||||
| lastCommittedOffset, err := getCommittedOffset(consumer, msg.TopicPartition.Partition) | ||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| log.Error("Failed to get committed offset", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| lastWorkspaceOffset := getLastWorkspaceOffset(lastSnapshot) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| offsetTracker := eventHanlder.OffsetTracker{ | ||||||||||||||||||||||||||||||||||||||||||||
| LastCommittedOffset: lastCommittedOffset, | ||||||||||||||||||||||||||||||||||||||||||||
| LastWorkspaceOffset: lastWorkspaceOffset, | ||||||||||||||||||||||||||||||||||||||||||||
| MessageOffset: messageOffset, | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| ws, err := handler.ListenAndRoute(ctx, msg, offsetTracker) | ||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| log.Error("Failed to route message", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if err := workspace.Save(ctx, storage, ws, snapshot); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| log.Error("Failed to save workspace", "workspaceID", ws.ID, "snapshotPath", snapshot.Path, "error", err) | ||||||||||||||||||||||||||||||||||||||||||||
| shouldSaveSnapshot := lastSnapshot == nil || lastSnapshot.Timestamp.Before(msg.Timestamp.Add(-time.Duration(MinSnapshotDistance)*time.Minute)) | ||||||||||||||||||||||||||||||||||||||||||||
| if shouldSaveSnapshot { | ||||||||||||||||||||||||||||||||||||||||||||
| snapshot := &db.WorkspaceSnapshot{ | ||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceID: ws.ID, | ||||||||||||||||||||||||||||||||||||||||||||
| Path: fmt.Sprintf("%s.gob", ws.ID), | ||||||||||||||||||||||||||||||||||||||||||||
| Timestamp: msg.Timestamp, | ||||||||||||||||||||||||||||||||||||||||||||
| Partition: int32(msg.TopicPartition.Partition), | ||||||||||||||||||||||||||||||||||||||||||||
| Offset: int64(msg.TopicPartition.Offset), | ||||||||||||||||||||||||||||||||||||||||||||
| NumPartitions: numPartitions, | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if err := workspace.Save(ctx, storage, ws, snapshot); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| log.Error("Failed to save workspace", "workspaceID", ws.ID, "snapshotPath", snapshot.Path, "error", err) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // Commit offset to Kafka | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.