api: getReaderSize() should honor seeked file descriptors. #681
Conversation
harshavardhana
force-pushed
the
reader-size
branch
from
e6465d3 to
b8b7560
Compare
harshavardhana
force-pushed
the
reader-size
branch
2 times, most recently
from
26457ec to
8a3d9a1
Compare
harshavardhana
force-pushed
the
reader-size
branch
from
8a3d9a1 to
e0b6cd0
Compare
harshavardhana
force-pushed
the
reader-size
branch
from
e0b6cd0 to
d92c6cf
Compare
api_functional_v4_test.go
Outdated
| t.Fatal("Error:", err) | ||
| } | ||
|
|
||
| objectName := fmt.Sprintf("test-file-%v", rand.Uint32()) |
There was a problem hiding this comment.
Why does objectName need a random suffix?
There was a problem hiding this comment.
Just for uniqueness.. Each test needs to be self contained and not overwrite each other with generic names.
| if err != nil { | ||
| t.Fatal("Error:", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why can't we write data here instead of closing and using io.WriteFile which would do its own Open and Close?
There was a problem hiding this comment.
We can.. let me add that.
api_functional_v4_test.go
Outdated
|
|
||
| objectName := fmt.Sprintf("test-file-%v", rand.Uint32()) | ||
|
|
||
| offset := 123 |
There was a problem hiding this comment.
Suggestion: we could have offset as length/2 (say). This ensures that offset would be valid independent of the length we choose. Having offset as an independent constant allows for situations in the future where offset may be greater than length etc. We have seen similar cases with our unit tests in the past, e.g index 13 was used to access a variably sized buffer.
There was a problem hiding this comment.
When would that happen in this case? this is a self contained test. But it is do-able to just do length/2.
There was a problem hiding this comment.
There is no issue with this PR. It is possible that constants for length and offset may be changed independently leading to an invalid test case accidentally. So, this is only a suggestion.
harshavardhana
force-pushed
the
reader-size
branch
from
d92c6cf to
ab6126a
Compare
In situations where a file has been Seeked, we need to start reading from the offset which it indeed happens. But our reader size calculation needs to honor this to be accurate. Fixes minio#680
harshavardhana
force-pushed
the
reader-size
branch
from
ab6126a to
cc83645
Compare
In situations where a file has been Seeked, we need to
start reading from the offset which it indeed happens.
But our reader size calculation needs to honor this to
be accurate.
Fixes #680