[BEAM-12988] [Playground] Add FileSystem service by AydarZaynutdinov · Pull Request #15645 · apache/beam

AydarZaynutdinov · 2021-10-04T12:21:21Z

Add FileSystemService and Java implementation of this service
This service contains methods to:

prepare file structure for compelled and executable files
delete all created folders for compelled and executable files
create an executable file with code

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

AydarZaynutdinov · 2021-10-04T12:22:23Z

R: @pabloem

AydarZaynutdinov · 2021-10-05T15:34:24Z

R: @damondouglas

codecov · 2021-10-05T19:07:16Z

Codecov Report

Merging #15645 (12f6ee5) into master (573beba) will increase coverage by 0.27%.
The diff coverage is 84.48%.

❗ Current head 12f6ee5 differs from pull request most recent head fb6d554. Consider uploading reports for the commit fb6d554 to get more accurate results

@@ Coverage Diff @@## master #15645 +/- ##
==========================================
+ Coverage 83.52% 83.80% +0.27% 
==========================================
Files 445 444 -1 Lines 61415 60474 -941 ==========================================
- Hits 51296 50679 -617 + Misses 10119 9795 -324

Impacted Files	Coverage Δ
sdks/python/apache_beam/dataframe/partitionings.py	`96.03% <ø> (ø)`
sdks/python/apache_beam/dataframe/transforms.py	`95.18% <ø> (ø)`
sdks/python/apache_beam/examples/avro_bitcoin.py	`0.00% <ø> (ø)`
...thon/apache_beam/examples/complete/autocomplete.py	`62.85% <ø> (ø)`
...n/apache_beam/examples/complete/game/game_stats.py	`45.74% <ø> (ø)`
...e_beam/examples/complete/game/hourly_team_score.py	`59.49% <ø> (ø)`
...apache_beam/examples/complete/game/leader_board.py	`52.63% <ø> (ø)`
...n/apache_beam/examples/complete/game/user_score.py	`62.79% <ø> (ø)`
...e_beam/examples/complete/top_wikipedia_sessions.py	`80.70% <ø> (ø)`
...che_beam/examples/flink/flink_streaming_impulse.py	`0.00% <0.00%> (ø)`
... and 162 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd0ac38...fb6d554. Read the comment docs.

damondouglas · 2021-10-05T18:36:04Z

@pabloem FYI I don't think this is a blocker but the code for playground/backend/internal/api is not part of this PR. It's a simple enough test that I'm ok moving forward and trusting everything will come together in time.

damondouglas · 2021-10-05T19:04:08Z

May we consider breaking up this interface to follow more of the typical interface to method 1:1 carnality we see with go? Brainstorming out loud, what do we think about the following? The following isn't complete but just a conversation starter. I removed the bool from the return signatures as it could be part of the error.
import "io/fs" type Setup interface { Create() (fs.FS, error) } type Teardown interface { Delete() error } type LifeCycle struct { sdk pb.Sdk root fs.FS folderGlobs []string } func New(ctx context.Context, sdk pb.Sdk) (*LifeCycle, error) { switch sdk { case pb.Sdk_SDK_JAVA: return &LifeCycle{ sdk: sdk, root: javaFS, // See java_fs.go folderGlobs: javaGlobs, }, nil default: return nil, fmt.Errorf("%s isn't supported now", sdk) } } func (lc *LifeCycle) Create() (fs.FS, error) { for _, k := range lc.folderGlobs { if err := MkdirAll( ... ); err != nil { ... } } } func (lc *LifeCycle) Delete() error { .... }

damondouglas · 2021-10-05T19:15:41Z

In addition to comments on the interface in playground/backend/internal/fs_tool/fs.go:
var ( javaGlobs = []string{ parentBaseFileFolder, javaBaseFileFolder, javaSrcFileFolder, javaBinFileFolder, } javaFs = os.DirFS(parentBaseFileFolder) )

damondouglas · 2021-10-05T19:21:18Z

The comments above may remove the need for a specific JavaFileSystemService struct.

damondouglas · 2021-10-06T14:37:10Z

This could be changed to:
folderGlobs []string

damondouglas · 2021-10-06T14:40:31Z

I imagine someone reading this in a web godoc would not know what folderGlobs is because it would be hidden.
// DeleteFolders deletes all previously provisioned folders

damondouglas · 2021-10-06T14:42:32Z

// CreateExecutableFile creates an executable file (i.e. file.java for the Java SDK).

damondouglas · 2021-10-06T14:50:15Z

Could we use os.WriteFile instead of ioutil.WriteFile? Please see https://golang.org/doc/go1.16#ioutil.
Also, may we hoist 0600 to the top of the file as a constant?

damondouglas

One more change and this is ready. Thank you for all this work. May we add the go.sum to this PR via go mod tidy?

damondouglas

@pabloem LGTM

pabloem · 2021-10-14T15:20:52Z

sorry - there's a merge conflict here - can you resolve so I'll merge? : )

Add LifeCycle; Add Java SDK environment of LifeCycle; Fix test description into README;

AydarZaynutdinov force-pushed the BEAM-12988-add-file-system-service branch 3 times, most recently from 19f91ae to ffe7c04Compare October 4, 2021 17:09

This was referenced Oct 4, 2021
[BEAM-12983] initial server akvelon/beam#26
Closed
[BEAM-12983] [Playground] Initial gRPC server #15656
Merged

AydarZaynutdinov force-pushed the BEAM-12988-add-file-system-service branch 2 times, most recently from e164461 to c37561bCompare October 5, 2021 14:34

AydarZaynutdinov force-pushed the BEAM-12988-add-file-system-service branch 2 times, most recently from 12f6ee5 to 91d96a9Compare October 5, 2021 18:58

damondouglas requested changes Oct 5, 2021
View reviewed changes

AydarZaynutdinov force-pushed the BEAM-12988-add-file-system-service branch from 2fafb3f to 8462bdfCompare October 6, 2021 12:42

damondouglas requested changes Oct 6, 2021
View reviewed changes

damondouglas approved these changes Oct 6, 2021
View reviewed changes

AydarZaynutdinov force-pushed the BEAM-12988-add-file-system-service branch 4 times, most recently from d6e9239 to ad33048Compare October 11, 2021 10:04

AydarZaynutdinov force-pushed the BEAM-12988-add-file-system-service branch 2 times, most recently from 63f24b0 to 8060c74Compare October 15, 2021 05:03

[BEAM-12988] [Playground]
fb6d554
Add LifeCycle; Add Java SDK environment of LifeCycle; Fix test description into README;

AydarZaynutdinov force-pushed the BEAM-12988-add-file-system-service branch from 8060c74 to fb6d554Compare October 15, 2021 05:25

pabloem merged commit bf91bd7 into apache:masterOct 18, 2021

AydarZaynutdinov deleted the BEAM-12988-add-file-system-service branch October 19, 2021 11:06

Uh oh!

Conversation

AydarZaynutdinov commented Oct 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

AydarZaynutdinov commented Oct 4, 2021

Uh oh!

AydarZaynutdinov commented Oct 5, 2021

Uh oh!

codecovBot commented Oct 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damondouglas left a comment

Choose a reason for hiding this comment

Uh oh!

damondouglas left a comment

Choose a reason for hiding this comment

Uh oh!

pabloem commented Oct 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AydarZaynutdinov commented Oct 4, 2021•
edited
Loading

`ValidatesRunner` compliance status (on master branch)

codecovBot commented Oct 5, 2021•
edited
Loading