You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/18 17:01:40 UTC

[GitHub] [beam] gonzojive opened a new pull request, #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

gonzojive opened a new pull request, #21942:
URL: https://github.com/apache/beam/pull/21942

   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:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Mention the appropriate issue in your description (for example: `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, comment `fixes #<ISSUE NUMBER>` instead.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159521015

   Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
   
   R: @jrmccluskey for label go.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] closed pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.
URL: https://github.com/apache/beam/pull/21942


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] asf-ci commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159515684

   Can one of the admins verify this patch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] jrmccluskey commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1177995626

   I'm cool with merging this once the branch conflicts are resolved 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] asf-ci commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159515686

   Can one of the admins verify this patch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1463756582

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] asf-ci commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159515682

   Can one of the admins verify this patch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] asf-ci commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159515683

   Can one of the admins verify this patch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] jrmccluskey commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159796503

   I would definitely like to see a unit test to demonstrate the functionality you're adding. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1453481949

   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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] jrmccluskey commented on a diff in pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on code in PR #21942:
URL: https://github.com/apache/beam/pull/21942#discussion_r907473286


##########
sdks/go/pkg/beam/io/filesystem/memfs/memory.go:
##########
@@ -156,9 +163,77 @@ type commitWriter struct {
 }
 
 func (w *commitWriter) Write(p []byte) (n int, err error) {
-	return w.buf.Write(p)
+	n, err = w.buf.Write(p)
+	if err != nil {
+		return n, err
+	}
+
+	w.instance.write(w.key, w.buf.Bytes())
+	return n, nil
 }
 
 func (w *commitWriter) Close() error {
-	return w.instance.write(w.key, w.buf.Bytes())
+	return nil
 }
+
+// bytesReader implements io.Reader, io.Seeker, io.Cloer for memfs "files."

Review Comment:
   ```suggestion
   // bytesReader implements io.Reader, io.Seeker, io.Closer for memfs "files."
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] asf-ci commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159515685

   Can one of the admins verify this patch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1177526834

   Reminder, please take a look at this pr: @jrmccluskey 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] jrmccluskey commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1162064669

   Having thought about it more, a runnable example demonstrating the usage you're proposing would also be really helpful (totally up to you.)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] gonzojive commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
gonzojive commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1166217882

   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).
   
   > Having thought about it more, a runnable example demonstrating the usage you're proposing would also be really helpful (totally up to you.)
   
   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1170213276

   > 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.
   
   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.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] gonzojive commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
gonzojive commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159793873

   I can add a test if desired.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] gonzojive commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
gonzojive commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1167766009

   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
   > <https://github.com/apache/beam/pull/21942#discussion_r907473286>:
   >
   > >  }
   >
   > +
   >
   > +// 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
   > <https://github.com/apache/beam/pull/21942#pullrequestreview-1020318674>,
   > or unsubscribe
   > <https://github.com/notifications/unsubscribe-auth/AAAUO55MD5QJ73ZOQ4LTK2DVRHM2DANCNFSM5ZFAQHDA>
   > .
   > You are receiving this because you authored the thread.Message ID:
   > ***@***.***>
   >
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] commented on pull request #21942: Go SDK: Update memory file system to return an io.ReadSeekCloser.

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21942:
URL: https://github.com/apache/beam/pull/21942#issuecomment-1159516232

   # [Codecov](https://codecov.io/gh/apache/beam/pull/21942?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#21942](https://codecov.io/gh/apache/beam/pull/21942?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bb8ef3a) into [master](https://codecov.io/gh/apache/beam/commit/525a169e6f807e301f1ac5e039645d4961da18d7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (525a169) will **decrease** coverage by `0.00%`.
   > The diff coverage is `60.00%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #21942      +/-   ##
   ==========================================
   - Coverage   73.98%   73.97%   -0.01%     
   ==========================================
     Files         702      702              
     Lines       92845    92849       +4     
   ==========================================
   + Hits        68687    68689       +2     
   - Misses      22903    22905       +2     
     Partials     1255     1255              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | go | `50.82% <60.00%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/21942?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [sdks/go/pkg/beam/io/filesystem/memfs/memory.go](https://codecov.io/gh/apache/beam/pull/21942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9pby9maWxlc3lzdGVtL21lbWZzL21lbW9yeS5nbw==) | `89.87% <60.00%> (-2.13%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/21942?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/21942?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [525a169...bb8ef3a](https://codecov.io/gh/apache/beam/pull/21942?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org