Push and Pull v2 #89
Push and Pull v2 #89
Conversation
bheni
requested review from
reltuk and
zachmu
and removed request for
reltuk
| rf, err := types.NewRef(cm.commitSt, ddb.db.Format()) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return datas.Pull(ctx, srcDB.db, ddb.db, rf, progChan) | ||
| if datas.CanUsePuller(srcDB.db) && datas.CanUsePuller(ddb.db) { | ||
| puller, err := datas.NewPuller(ctx, tempDir, 256*1024, srcDB.db, ddb.db, rf.TargetHash()) |
| return err | ||
| } | ||
|
|
||
| if puller == nil { |
There was a problem hiding this comment.
This is a strange interface. Can't the Pull method respond to a nil pointer and return nil?
There was a problem hiding this comment.
updated the interface. Using a new ErrDBUpToDate returned by NewPuller
| @@ -150,9 +152,51 @@ func (dcs *DoltChunkStore) Get(ctx context.Context, h hash.Hash) (chunks.Chunk, | |||
| } | |||
| } | |||
|
|
|||
| func (dcs *DoltChunkStore) GetMany(ctx context.Context, hashes hash.HashSet, foundChunks chan *chunks.Chunk) error { | |||
| ae := atomicerr.New() | |||
There was a problem hiding this comment.
Seems like this doesn't really need to be an atomic error, since there is only one writer
There was a problem hiding this comment.
the error is modified in a go routine and then checked on the main thread.
| ch, err := chable.ToChunk() | ||
|
|
||
| if err != nil { | ||
| return map[hash.Hash]int{}, err |
There was a problem hiding this comment.
Aren't maps ref types? If so this should be nil, err
| // NewSink still needs to be implemented in order to write to a DoltChunkStore using the TableFileStore interface | ||
| func (dcs *DoltChunkStore) NewSink(ctx context.Context, fileId string, numChunks int) (nbs.WriteCloserWithContext, error) { | ||
| panic("Not implemented") | ||
| // WriteTableFile returns a writer for a new table file. When the writer is closed the table file is persisted |
go/store/nbs/byte_sink.go
Outdated
| return iohelp.WriteAll(wr, sink.buff[:sink.pos]) | ||
| } | ||
|
|
||
| // FlushToFile writes all teh data that was written to the file to a file at the provided location. |
go/store/nbs/byte_sink.go
Outdated
| return iohelp.WriteAll(wr, sink.blocks...) | ||
| } | ||
|
|
||
| // FlushToFile writes all teh data that was written to the file to a file at the provided location. |
| sink.ae.SetIfError(err) | ||
| } | ||
|
|
||
| err = sink.wr.Close() |
There was a problem hiding this comment.
This stomps any previously set error
There was a problem hiding this comment.
that's fine. It's just updating the ae which won't change if it's already set.
go/store/nbs/byte_sink.go
Outdated
| return err | ||
| } | ||
|
|
||
| // FlushToFile writes all teh data that was written to the file to a file at the provided location. |
| repeated ChunkTableInfo chunk_table_info = 3; | ||
| } | ||
|
|
||
| message AddTableFilesResponse { |
There was a problem hiding this comment.
I think it's more customary to return Empty or throw an exception, rather than make a client check a success flag.
There was a problem hiding this comment.
yes. will fix this and commit at a later time.
No description provided.