Go SDK: Update memory file system to return an io.ReadSeekCloser. #21942
Conversation
When writing tests for spittable DoFns that need to seek within files, it's helpful to use memfs. Before this change, memfs returned an io.ReadCloser that did not implement io.Seeker.
|
Can one of the admins verify this patch? |
4 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
Codecov Report
@@ Coverage Diff @@
## master #21942 +/- ##
==========================================
+ Coverage 73.98% 74.02% +0.04%
==========================================
Files 702 703 +1
Lines 92845 92996 +151
==========================================
+ Hits 68687 68843 +156
+ Misses 22903 22881 -22
- Partials 1255 1272 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label go. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
I can add a test if desired. |
|
I would definitely like to see a unit test to demonstrate the functionality you're adding. |
|
Having thought about it more, a runnable example demonstrating the usage you're proposing would also be really helpful (totally up to you.) |
… writes." This reverts commit 2c2355951f308dbcdaefe4ddca38fb9d74f3a954.
…ltaneous writes."" This reverts commit 392e740.
|
I added tests and revised the implementation. I noticed there is another gotcha with memfs: open readers don't see the results of writers. I addressed this in the code as well and added tests for it (that break before the commits).
Done. Please see the updated implementation of TextIO that uses Seek to dramatically reduce the total # of read bytes. When making a change like that in TextIO, it's helpful if memfs behaves very similarly to a local file system and also implements io.Seeker. |
gonzojive
force-pushed
the
gosdk-memfs-seek
branch
from
4e06166 to
8e96cbb
Compare
gonzojive
force-pushed
the
gosdk-memfs-seek
branch
from
8e96cbb to
6d5935a
Compare
jrmccluskey
left a comment
jrmccluskey
left a comment
There was a problem hiding this comment.
LGTM, going to loop in @lostluck for a second set of eyes.
| return nil | ||
| } | ||
|
|
||
| // bytesReader implements io.Reader, io.Seeker, io.Cloer for memfs "files." |
There was a problem hiding this comment.
| // bytesReader implements io.Reader, io.Seeker, io.Cloer for memfs "files." | |
| // bytesReader implements io.Reader, io.Seeker, io.Closer for memfs "files." |
|
One idea is to update the interface of the memfa package to expose bytes
read rather than copying a lot of the memfs implementation into the TextIO
test.
…On Mon, Jun 27, 2022, 10:00 AM Jack McCluskey ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM, going to loop in @lostluck <https://github.com/lostluck> for a
second set of eyes.
------------------------------
In sdks/go/pkg/beam/io/filesystem/memfs/memory.go
<#21942 (comment)>:
> }
+
+// bytesReader implements io.Reader, io.Seeker, io.Cloer for memfs "files."
⬇️ Suggested change
-// bytesReader implements io.Reader, io.Seeker, io.Cloer for memfs "files."
+// bytesReader implements io.Reader, io.Seeker, io.Closer for memfs "files."
—
Reply to this email directly, view it on GitHub
<#21942 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUO55MD5QJ73ZOQ4LTK2DVRHM2DANCNFSM5ZFAQHDA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I like that better than duplicating logic into the textio test. Memfs is at best intended for simple tests and demos, so this would be an appropriate extension. |
|
Reminder, please take a look at this pr: @jrmccluskey |
|
I'm cool with merging this once the branch conflicts are resolved |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
4 participants
When writing tests for spittable DoFns that need to seek within files, it's
helpful to use memfs. Before this change, memfs returned an io.ReadCloser that
did not implement io.Seeker.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.