-
Notifications
You must be signed in to change notification settings - Fork 205
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
Add stream IDs, proper storage prefixing, recovering from crashes. #177
Changes from all commits
c47552c
35b6f69
8733684
508ab72
89f2001
d6f541b
701831e
d949e63
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 |
---|---|---|
|
@@ -61,19 +61,19 @@ type IntermediateRecordStore interface { | |
type PullEngine struct { | ||
irs IntermediateRecordStore | ||
source RecordStream | ||
sourceStoragePrefix []byte | ||
lastCommittedWatermark time.Time | ||
watermarkSource WatermarkSource | ||
storage storage.Storage | ||
streamID *StreamID | ||
} | ||
|
||
func NewPullEngine(irs IntermediateRecordStore, storage storage.Storage, source RecordStream, sourceStoragePrefix []byte, watermarkSource WatermarkSource) *PullEngine { | ||
func NewPullEngine(irs IntermediateRecordStore, storage storage.Storage, source RecordStream, streamID *StreamID, watermarkSource WatermarkSource) *PullEngine { | ||
return &PullEngine{ | ||
irs: irs, | ||
storage: storage, | ||
source: source, | ||
sourceStoragePrefix: sourceStoragePrefix, | ||
watermarkSource: watermarkSource, | ||
irs: irs, | ||
storage: storage, | ||
source: source, | ||
streamID: streamID, | ||
watermarkSource: watermarkSource, | ||
} | ||
} | ||
|
||
|
@@ -127,43 +127,46 @@ func (engine *PullEngine) Run(ctx context.Context) { | |
} else if err != nil { | ||
tx.Abort() | ||
log.Println(err) | ||
return // TODO: Error propagation? Add this to the underlying queue as an ErrorElement? How to do this well? | ||
return // TODO: Error propagation? Add this to the underlying queue as an ErrorElement? How to do this well? Send it to the underlying IRS like a watermark? | ||
} | ||
} | ||
} | ||
|
||
func (engine *PullEngine) loop(ctx context.Context, tx storage.StateTransaction) error { | ||
sourcePrefixedTx := tx.WithPrefix(engine.sourceStoragePrefix) | ||
// This is a transaction prefixed with the current node StreamID, | ||
// which should be used for all storage operations of this node. | ||
// Source streams will get the raw, non-prefixed, transaction. | ||
prefixedTx := tx.WithPrefix(engine.streamID.AsPrefix()) | ||
|
||
watermark, err := engine.watermarkSource.GetWatermark(ctx, sourcePrefixedTx) | ||
watermark, err := engine.watermarkSource.GetWatermark(ctx, tx) | ||
if err != nil { | ||
return errors.Wrap(err, "couldn't get current watermark from source") | ||
} | ||
if watermark.After(engine.lastCommittedWatermark) { | ||
err := engine.irs.UpdateWatermark(ctx, tx, watermark) | ||
err := engine.irs.UpdateWatermark(ctx, prefixedTx, watermark) | ||
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. Same thing here - these changes need commenting in my opinion 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. I've added necessary comments. |
||
if err != nil { | ||
return errors.Wrap(err, "couldn't update watermark in intermediate record store") | ||
} | ||
engine.lastCommittedWatermark = watermark // TODO: last _commited_ watermark :( this is not committed | ||
return nil | ||
} | ||
|
||
record, err := engine.source.Next(storage.InjectStateTransaction(ctx, sourcePrefixedTx)) | ||
record, err := engine.source.Next(storage.InjectStateTransaction(ctx, tx)) | ||
if err != nil { | ||
if err == ErrEndOfStream { | ||
err := engine.irs.UpdateWatermark(ctx, tx, maxWatermark) | ||
err := engine.irs.UpdateWatermark(ctx, prefixedTx, maxWatermark) | ||
if err != nil { | ||
return errors.Wrap(err, "couldn't mark end of stream max watermark in intermediate record store") | ||
} | ||
err = engine.irs.MarkEndOfStream(ctx, tx) | ||
err = engine.irs.MarkEndOfStream(ctx, prefixedTx) | ||
if err != nil { | ||
return errors.Wrap(err, "couldn't mark end of stream in intermediate record store") | ||
} | ||
return ErrEndOfStream | ||
} | ||
return errors.Wrap(err, "couldn't get next record") | ||
} | ||
err = engine.irs.AddRecord(ctx, tx, 0, record) | ||
err = engine.irs.AddRecord(ctx, prefixedTx, 0, record) | ||
if err != nil { | ||
return errors.Wrap(err, "couldn't add record to intermediate record store") | ||
} | ||
|
@@ -173,7 +176,9 @@ func (engine *PullEngine) loop(ctx context.Context, tx storage.StateTransaction) | |
|
||
func (engine *PullEngine) Next(ctx context.Context) (*Record, error) { | ||
tx := storage.GetStateTransactionFromContext(ctx) | ||
rec, err := engine.irs.Next(ctx, tx) | ||
prefixedTx := tx.WithPrefix(engine.streamID.AsPrefix()) | ||
|
||
rec, err := engine.irs.Next(ctx, prefixedTx) | ||
if err != nil { | ||
if err == ErrEndOfStream { | ||
return nil, ErrEndOfStream | ||
|
@@ -184,7 +189,9 @@ func (engine *PullEngine) Next(ctx context.Context) (*Record, error) { | |
} | ||
|
||
func (engine *PullEngine) GetWatermark(ctx context.Context, tx storage.StateTransaction) (time.Time, error) { | ||
return engine.irs.GetWatermark(ctx, tx) | ||
prefixedTx := tx.WithPrefix(engine.streamID.AsPrefix()) | ||
|
||
return engine.irs.GetWatermark(ctx, prefixedTx) | ||
} | ||
|
||
func (engine *PullEngine) Close() error { | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
syntax = "proto3"; | ||
package execution; | ||
option go_package = "github.com/cube2222/octosql/execution"; | ||
|
||
// StreamID is a unique identifier for a RecordStream node. | ||
// This StreamID should prefix all state storage keys this node uses. | ||
message StreamID { | ||
string id = 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a very subtle change, yet there is nothing that explains it. Could you add a comment, as to explain why the watermark is suddenly created using the original transaction, and not the prefixed one, as was in the previous version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added necessary comments.