add copy to adapter #797
add copy to adapter #797
Conversation
Codecov Report
@@ Coverage Diff @@
## master #797 +/- ##
==========================================
- Coverage 42.08% 41.62% -0.47%
==========================================
Files 134 133 -1
Lines 10434 10384 -50
==========================================
- Hits 4391 4322 -69
- Misses 5461 5483 +22
+ Partials 582 579 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Cool, thanks!
We do need to test this, understandably such integration testing is not part of this PR. So please do open an issue to do that.
The only required change is to pass ObjectPointers in the adapter interface, just like all other methods do. That also means getting rid of the icky resolve calls in the adapter I think.
| @@ -56,6 +56,7 @@ type Adapter interface { | |||
| GetRange(obj ObjectPointer, startPosition int64, endPosition int64) (io.ReadCloser, error) | |||
| GetProperties(obj ObjectPointer) (Properties, error) | |||
| Remove(obj ObjectPointer) error | |||
| Copy(sourceObj, destinationObj ObjectPointer) error | |||
There was a problem hiding this comment.
Don't these need to be ObjectPointers? Otherwise how do we handle namespaces?
There was a problem hiding this comment.
They are ObjectPointers
There was a problem hiding this comment.
Aaah, evidently I can copy but I cannot read.
block/local/adapter.go
Outdated
| _ = destinationFile.Close() | ||
| }() | ||
| source := l.getPath(sourceObj.Identifier) | ||
| sourceFile, err := os.OpenFile(source, os.O_RDONLY, 0755) |
There was a problem hiding this comment.
Why not os.Open? (You anyway pass weird permissions, that are then ignored.)
| @@ -93,6 +93,27 @@ func (l *Adapter) Remove(obj block.ObjectPointer) error { | |||
| return os.Remove(p) | |||
| } | |||
|
|
|||
| func (l *Adapter) Copy(sourceObj, destinationObj block.ObjectPointer) error { | |||
| dest := l.getPath(destinationObj.Identifier) | |||
| destinationFile, err := os.Create(dest) | |||
There was a problem hiding this comment.
Create after opening the source - opening the source is a cheaper operation.
| return err | ||
| } | ||
| defer func() { | ||
| _ = destinationFile.Close() |
There was a problem hiding this comment.
- This ignores errors.
- This leaves a bad (empty) file when the copy fails (or even if the source cannot be opened, because of create-before-open above). It should instead delete it.
| @@ -56,6 +56,7 @@ type Adapter interface { | |||
| GetRange(obj ObjectPointer, startPosition int64, endPosition int64) (io.ReadCloser, error) | |||
| GetProperties(obj ObjectPointer) (Properties, error) | |||
| Remove(obj ObjectPointer) error | |||
| Copy(sourceObj, destinationObj ObjectPointer) error | |||
There was a problem hiding this comment.
Aaah, evidently I can copy but I cannot read.
Co-authored-by: arielshaqed <ariels@treeverse.io>
change order between create and open in local/adapter
No description provided.